[PATCH v2] erofs-utils: fix the erofs_io_pread and erofs_io_pwrite

[Gao Xiang]

On 2024/6/18 17:38, Hongzhen Luo wrote:
> When `vf->ops` is not null, `vf->ops->pread` returns the
> number of bytes successfully read, which is inconsistent
> with the semantics of `erofs_io_pread`. Similar situation
> occurs in `erofs_io_pwrite`. This fixes this issue.
> 
> This also fixes `erofs_dev_close`.
> 
> Signed-off-by: Hongzhen Luo <hongzhen at linux.alibaba.com>
> ---
> v1: https://lore.kernel.org/all/20240617023433.3446706-1-hongzhen@linux.alibaba.com/
> ---
>   include/erofs/internal.h |  5 ++++-
>   lib/io.c                 | 31 +++++++++++++++++++++++--------
>   2 files changed, 27 insertions(+), 9 deletions(-)
> 
> diff --git a/include/erofs/internal.h b/include/erofs/internal.h
> index f8a01ce..ed1fc92 100644
> --- a/include/erofs/internal.h
> +++ b/include/erofs/internal.h
> @@ -460,7 +460,10 @@ ssize_t erofs_dev_read(struct erofs_sb_info *sbi, int device_id,
>   static inline int erofs_dev_write(struct erofs_sb_info *sbi, const void *buf,
>   				  u64 offset, size_t len)
>   {
> -	return erofs_io_pwrite(&sbi->bdev, buf, offset, len);
> +	ssize_t ret;
> +
> +	ret = erofs_io_pwrite(&sbi->bdev, buf, offset, len);
> +	return (size_t)ret == len ? 0 : (int)ret;
>   }
>   
>   static inline int erofs_dev_fillzero(struct erofs_sb_info *sbi, u64 offset,
> diff --git a/lib/io.c b/lib/io.c
> index c523f00..0149d3b 100644
> --- a/lib/io.c
> +++ b/lib/io.c
> @@ -30,6 +30,7 @@ ssize_t erofs_io_pwrite(struct erofs_vfile *vf, const void *buf,
>   			u64 pos, size_t len)
>   {
>   	ssize_t ret;
> +	size_t saved_len = len;
>   
>   	if (unlikely(cfg.c_dry_run))
>   		return 0;
> @@ -53,7 +54,7 @@ ssize_t erofs_io_pwrite(struct erofs_vfile *vf, const void *buf,
>   		pos += ret;
>   	} while (len);
>   
> -	return 0;
> +	return saved_len;
>   }
>   
>   int erofs_io_fsync(struct erofs_vfile *vf)
> @@ -79,6 +80,7 @@ ssize_t erofs_io_fallocate(struct erofs_vfile *vf, u64 offset,
>   {
>   	static const char zero[EROFS_MAX_BLOCK_SIZE] = {0};
>   	ssize_t ret;
> +	ssize_t wret;
why not directly using ret here?

>   
>   	if (unlikely(cfg.c_dry_run))
>   		return 0;
> @@ -92,13 +94,16 @@ ssize_t erofs_io_fallocate(struct erofs_vfile *vf, u64 offset,
>   		return 0;
>   #endif
>   	while (len > EROFS_MAX_BLOCK_SIZE) {
> -		ret = erofs_io_pwrite(vf, zero, offset, EROFS_MAX_BLOCK_SIZE);
> +		wret = erofs_io_pwrite(vf, zero, offset, EROFS_MAX_BLOCK_SIZE);
> +		ret = wret == EROFS_MAX_BLOCK_SIZE ? 0 : wret;
>   		if (ret)
>   			return ret;
>   		len -= EROFS_MAX_BLOCK_SIZE;
>   		offset += EROFS_MAX_BLOCK_SIZE;
>   	}
> -	return erofs_io_pwrite(vf, zero, offset, len);
> +	wret = erofs_io_pwrite(vf, zero, offset, len);
> +	ret = wret == len ? 0 : wret;

	


> +	return ret;
>   }
>   
>   int erofs_io_ftruncate(struct erofs_vfile *vf, u64 length)
> @@ -126,6 +131,7 @@ int erofs_io_ftruncate(struct erofs_vfile *vf, u64 length)
>   ssize_t erofs_io_pread(struct erofs_vfile *vf, void *buf, u64 pos, size_t len)
>   {
>   	ssize_t ret;
> +	size_t saved_len = len;

I really think you need to record the data read and

You don't address:
                         if (!ret) {
                                 erofs_info("reach EOF of device");
                                 memset(buf, 0, len);
                                 return 0;
                         }

>   
>   	if (unlikely(cfg.c_dry_run))
>   		return 0;
> @@ -156,7 +162,8 @@ ssize_t erofs_io_pread(struct erofs_vfile *vf, void *buf, u64 pos, size_t len)
>   		len -= ret;
>   		buf += ret;
>   	} while (len);
> -	return 0;
> +
> +	return saved_len;
>   }
>   
>   static int erofs_get_bdev_size(int fd, u64 *bytes)
> @@ -287,7 +294,8 @@ out:
>   
>   void erofs_dev_close(struct erofs_sb_info *sbi)
>   {
> -	close(sbi->bdev.fd);
> +	if (!sbi->bdev.ops)
> +		close(sbi->bdev.fd);
>   	free(sbi->devname);
>   	sbi->devname = NULL;
>   	sbi->bdev.fd = -1;
> @@ -320,11 +328,18 @@ int erofs_blob_open_ro(struct erofs_sb_info *sbi, const char *dev)
>   ssize_t erofs_dev_read(struct erofs_sb_info *sbi, int device_id,
>   		       void *buf, u64 offset, size_t len)
>   {
> -	if (device_id)
> -		return erofs_io_pread(&((struct erofs_vfile) {
> +	ssize_t ret;
> +
> +	if (device_id) {
> +		ret = erofs_io_pread(&((struct erofs_vfile) {
>   				.fd = sbi->blobfd[device_id - 1],
>   			}), buf, offset, len);
> -	return erofs_io_pread(&sbi->bdev, buf, offset, len);
> +		return ret == len ? 0 : ret;

What's the meaning if ret > 0?

> +
> +	}
> +
> +	ret =  erofs_io_pread(&sbi->bdev, buf, offset, len);
> +	return ret == len ? 0 : ret;

Same here.

Thanks,
Gao Xiang