* [PATCH] target/file: fix inclusive vfs_fsync_range() end
@ 2014-10-06 23:40 Zach Brown
2014-10-07 6:39 ` Christoph Hellwig
2014-10-08 6:11 ` Nicholas A. Bellinger
0 siblings, 2 replies; 4+ messages in thread
From: Zach Brown @ 2014-10-06 23:40 UTC (permalink / raw)
To: Nicholas A. Bellinger, linux-scsi, target-devel, linux-kernel
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] target/file: fix inclusive vfs_fsync_range() end
2014-10-06 23:40 [PATCH] target/file: fix inclusive vfs_fsync_range() end Zach Brown
@ 2014-10-07 6:39 ` Christoph Hellwig
2014-10-07 19:12 ` Zach Brown
2014-10-08 6:11 ` Nicholas A. Bellinger
1 sibling, 1 reply; 4+ messages in thread
From: Christoph Hellwig @ 2014-10-07 6:39 UTC (permalink / raw)
To: Zach Brown; +Cc: Nicholas A. Bellinger, linux-scsi, target-devel, linux-kernel
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?
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] target/file: fix inclusive vfs_fsync_range() end
2014-10-07 6:39 ` Christoph Hellwig
@ 2014-10-07 19:12 ` Zach Brown
0 siblings, 0 replies; 4+ messages in thread
From: Zach Brown @ 2014-10-07 19:12 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Nicholas A. Bellinger, linux-scsi, target-devel, linux-kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] target/file: fix inclusive vfs_fsync_range() end
2014-10-06 23:40 [PATCH] target/file: fix inclusive vfs_fsync_range() end Zach Brown
2014-10-07 6:39 ` Christoph Hellwig
@ 2014-10-08 6:11 ` Nicholas A. Bellinger
1 sibling, 0 replies; 4+ messages in thread
From: Nicholas A. Bellinger @ 2014-10-08 6:11 UTC (permalink / raw)
To: Zach Brown; +Cc: linux-scsi, target-devel, linux-kernel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-10-08 6:11 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-10-06 23:40 [PATCH] target/file: fix inclusive vfs_fsync_range() end Zach Brown
2014-10-07 6:39 ` Christoph Hellwig
2014-10-07 19:12 ` Zach Brown
2014-10-08 6:11 ` Nicholas A. Bellinger
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).