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 <[email protected]>
---
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
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?
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
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 <[email protected]>
> ---
> 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