[PATCH] target/file: fix inclusive vfs_fsync_range() end

[PATCH] target/file: fix inclusive vfs_fsync_range() end

[Zach Brown]

Both of the file target's calls to vfs_fsync_range() got the end offset
off by one.  The range is inclusive, not exclusive.  It would sync a bit
more data than was required.

The sync path already tested the length of the range and fell back to
LLONG_MAX so I copied that pattern in the rw path.

This is untested. I found the errors by inspection while following other
code.

Signed-off-by: Zach Brown <zab@zabbo.net>
---
 drivers/target/target_core_file.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index 7d6cdda..176588f 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
@@ -415,7 +415,7 @@ fd_execute_sync_cache(struct se_cmd *cmd)
 	} else {
 		start = cmd->t_task_lba * dev->dev_attrib.block_size;
 		if (cmd->data_length)
-			end = start + cmd->data_length;
+			end = start + cmd->data_length - 1;
 		else
 			end = LLONG_MAX;
 	}
@@ -680,7 +680,12 @@ fd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 			struct fd_dev *fd_dev = FD_DEV(dev);
 			loff_t start = cmd->t_task_lba *
 				dev->dev_attrib.block_size;
-			loff_t end = start + cmd->data_length;
+			loff_t end;
+
+			if (cmd->data_length)
+				end = start + cmd->data_length - 1;
+			else
+				end = LLONG_MAX;
 
 			vfs_fsync_range(fd_dev->fd_file, start, end, 1);
 		}
-- 
1.9.3

Re: [PATCH] target/file: fix inclusive vfs_fsync_range() end

[Christoph Hellwig]

On Mon, Oct 06, 2014 at 04:40:13PM -0700, Zach Brown wrote:
> Both of the file target's calls to vfs_fsync_range() got the end offset
> off by one.  The range is inclusive, not exclusive.  It would sync a bit
> more data than was required.
> 
> The sync path already tested the length of the range and fell back to
> LLONG_MAX so I copied that pattern in the rw path.
> 
> This is untested. I found the errors by inspection while following other
> code.

Maybe it's time to move vfs_fsync_range to a more normal calling
convention?

Re: [PATCH] target/file: fix inclusive vfs_fsync_range() end

[Zach Brown]

On Mon, Oct 06, 2014 at 11:39:45PM -0700, Christoph Hellwig wrote:
> On Mon, Oct 06, 2014 at 04:40:13PM -0700, Zach Brown wrote:
> > Both of the file target's calls to vfs_fsync_range() got the end offset
> > off by one.  The range is inclusive, not exclusive.  It would sync a bit
> > more data than was required.
> > 
> > The sync path already tested the length of the range and fell back to
> > LLONG_MAX so I copied that pattern in the rw path.
> > 
> > This is untested. I found the errors by inspection while following other
> > code.
> 
> Maybe it's time to move vfs_fsync_range to a more normal calling
> convention?

Yeah, I wanted to just fix the bugs before going too far.  

The current interface does seem like a bad fit.  3 of the 5 non-core
callers got it wrong.  They all generate start,end from off,count and
fall back to LLONG_MAX.

I'll put something together if no one beats me to it.

- z

Re: [PATCH] target/file: fix inclusive vfs_fsync_range() end

[Nicholas A. Bellinger]

On Mon, 2014-10-06 at 16:40 -0700, Zach Brown wrote:
> Both of the file target's calls to vfs_fsync_range() got the end offset
> off by one.  The range is inclusive, not exclusive.  It would sync a bit
> more data than was required.
> 
> The sync path already tested the length of the range and fell back to
> LLONG_MAX so I copied that pattern in the rw path.
> 
> This is untested. I found the errors by inspection while following other
> code.
> 
> Signed-off-by: Zach Brown <zab@zabbo.net>
> ---
>  drivers/target/target_core_file.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
> index 7d6cdda..176588f 100644
> --- a/drivers/target/target_core_file.c
> +++ b/drivers/target/target_core_file.c
> @@ -415,7 +415,7 @@ fd_execute_sync_cache(struct se_cmd *cmd)
>  	} else {
>  		start = cmd->t_task_lba * dev->dev_attrib.block_size;
>  		if (cmd->data_length)
> -			end = start + cmd->data_length;
> +			end = start + cmd->data_length - 1;
>  		else
>  			end = LLONG_MAX;
>  	}
> @@ -680,7 +680,12 @@ fd_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
>  			struct fd_dev *fd_dev = FD_DEV(dev);
>  			loff_t start = cmd->t_task_lba *
>  				dev->dev_attrib.block_size;
> -			loff_t end = start + cmd->data_length;
> +			loff_t end;
> +
> +			if (cmd->data_length)
> +				end = start + cmd->data_length - 1;
> +			else
> +				end = LLONG_MAX;
>  
>  			vfs_fsync_range(fd_dev->fd_file, start, end, 1);
>  		}

Nice catch Zach.  Applying to target-pending/for-next now.

Thank you,

--nab