* [PATCH] fuse: fix possible write position calculation error
@ 2021-11-03 1:15 Peng Hao
2021-11-04 12:17 ` Miklos Szeredi
0 siblings, 1 reply; 6+ messages in thread
From: Peng Hao @ 2021-11-03 1:15 UTC (permalink / raw)
To: miklos; +Cc: linux-fsdevel
The 'written' that generic_file_direct_write return through
filemap_write_and_wait_range is not necessarily sequential,
and its iocb->ki_pos has not been updated.
Signed-off-by: Peng Hao <flyingpeng@tencent.com>
---
fs/fuse/file.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 26730e699d68..52aaa1fb484d 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1314,14 +1314,12 @@ static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from)
goto out;
if (iocb->ki_flags & IOCB_DIRECT) {
- loff_t pos = iocb->ki_pos;
+ loff_t pos;
written = generic_file_direct_write(iocb, from);
if (written < 0 || !iov_iter_count(from))
goto out;
- pos += written;
-
- written_buffered = fuse_perform_write(iocb, mapping, from, pos);
+ written_buffered = fuse_perform_write(iocb, mapping, from, pos = iocb->ki_pos);
if (written_buffered < 0) {
err = written_buffered;
goto out;
--
2.27.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] fuse: fix possible write position calculation error
2021-11-03 1:15 [PATCH] fuse: fix possible write position calculation error Peng Hao
@ 2021-11-04 12:17 ` Miklos Szeredi
2021-11-05 7:43 ` Hao Peng
0 siblings, 1 reply; 6+ messages in thread
From: Miklos Szeredi @ 2021-11-04 12:17 UTC (permalink / raw)
To: Peng Hao; +Cc: linux-fsdevel
On Wed, 3 Nov 2021 at 02:15, Peng Hao <flyingpenghao@gmail.com> wrote:
>
> The 'written' that generic_file_direct_write return through
> filemap_write_and_wait_range is not necessarily sequential,
> and its iocb->ki_pos has not been updated.
I don't see the bug, but maybe I'm missing something. Can you please
explain in detail?
The patch looks good as a cleanup, but I'd very much like to know
first if it fixes a bug or not.
Thanks,
Miklos
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fuse: fix possible write position calculation error
2021-11-04 12:17 ` Miklos Szeredi
@ 2021-11-05 7:43 ` Hao Peng
2021-11-10 10:40 ` Miklos Szeredi
0 siblings, 1 reply; 6+ messages in thread
From: Hao Peng @ 2021-11-05 7:43 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: linux-fsdevel
On Thu, Nov 4, 2021 at 8:18 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Wed, 3 Nov 2021 at 02:15, Peng Hao <flyingpenghao@gmail.com> wrote:
> >
> > The 'written' that generic_file_direct_write return through
> > filemap_write_and_wait_range is not necessarily sequential,
> > and its iocb->ki_pos has not been updated.
>
> I don't see the bug, but maybe I'm missing something. Can you please
> explain in detail?
>
I think we shouldn't add "written" to variable pos.
generic_file_direct_write:
....
written = filemap_write_and_wait_range(mapping, pos,
pos + write_len - 1);
if (written) //the number of writes here reflects the
amount of writeback data
// in the previous page cache, and
iocb->ki_pos do not change and
// no data in iocb is written.
goto out;
...
out:
return written;
> The patch looks good as a cleanup, but I'd very much like to know
> first if it fixes a bug or not.
>
When the direct read & write and cache read & write of different processes,
there is some synchronization in the user mode, but it is still found
that there
is data out of synchronization. Just analyze that there may be problems here.
Thanks.
> Thanks,
> Miklos
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] fuse: fix possible write position calculation error
2021-11-05 7:43 ` Hao Peng
@ 2021-11-10 10:40 ` Miklos Szeredi
2021-11-11 1:40 ` Hao Peng
0 siblings, 1 reply; 6+ messages in thread
From: Miklos Szeredi @ 2021-11-10 10:40 UTC (permalink / raw)
To: Hao Peng; +Cc: linux-fsdevel
On Fri, 5 Nov 2021 at 08:44, Hao Peng <flyingpenghao@gmail.com> wrote:
>
> On Thu, Nov 4, 2021 at 8:18 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Wed, 3 Nov 2021 at 02:15, Peng Hao <flyingpenghao@gmail.com> wrote:
> > >
> > > The 'written' that generic_file_direct_write return through
> > > filemap_write_and_wait_range is not necessarily sequential,
> > > and its iocb->ki_pos has not been updated.
> >
> > I don't see the bug, but maybe I'm missing something. Can you please
> > explain in detail?
> >
> I think we shouldn't add "written" to variable pos.
> generic_file_direct_write:
> ....
> written = filemap_write_and_wait_range(mapping, pos,
> pos + write_len - 1);
> if (written) //the number of writes here reflects the
> amount of writeback data
No. It's actually an error code in this case.
It is confusing, though, so I guess cleaning this up (e.g. rename
"written" to "retval") would make sense.
Thanks,
Miklos
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] fuse: fix possible write position calculation error
2021-11-10 10:40 ` Miklos Szeredi
@ 2021-11-11 1:40 ` Hao Peng
0 siblings, 0 replies; 6+ messages in thread
From: Hao Peng @ 2021-11-11 1:40 UTC (permalink / raw)
To: Miklos Szeredi; +Cc: linux-fsdevel
On Wed, Nov 10, 2021 at 6:40 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Fri, 5 Nov 2021 at 08:44, Hao Peng <flyingpenghao@gmail.com> wrote:
> >
> > On Thu, Nov 4, 2021 at 8:18 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > On Wed, 3 Nov 2021 at 02:15, Peng Hao <flyingpenghao@gmail.com> wrote:
> > > >
> > > > The 'written' that generic_file_direct_write return through
> > > > filemap_write_and_wait_range is not necessarily sequential,
> > > > and its iocb->ki_pos has not been updated.
> > >
> > > I don't see the bug, but maybe I'm missing something. Can you please
> > > explain in detail?
> > >
> > I think we shouldn't add "written" to variable pos.
> > generic_file_direct_write:
> > ....
> > written = filemap_write_and_wait_range(mapping, pos,
> > pos + write_len - 1);
> > if (written) //the number of writes here reflects the
> > amount of writeback data
>
> No. It's actually an error code in this case.
>
> It is confusing, though, so I guess cleaning this up (e.g. rename
> "written" to "retval") would make sense.
>
oh,sorry.
I misunderstood. I should deeply analyze the function called below.
> Thanks,
> Miklos
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] fuse: fix possible write position calculation error
@ 2021-10-26 2:34 Peng Hao
0 siblings, 0 replies; 6+ messages in thread
From: Peng Hao @ 2021-10-26 2:34 UTC (permalink / raw)
To: miklos; +Cc: linux-fsdevel
The 'written' that generic_file_direct_write return through
filemap_write_and_wait_range is not necessarily sequential,
and its iocb->ki_pos has not been updated.
Signed-off-by: Peng Hao <flyingpeng@tencent.com>
---
fs/fuse/file.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 26730e699d68..52aaa1fb484d 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1314,14 +1314,12 @@ static ssize_t fuse_cache_write_iter(struct kiocb *iocb, struct iov_iter *from)
goto out;
if (iocb->ki_flags & IOCB_DIRECT) {
- loff_t pos = iocb->ki_pos;
+ loff_t pos;
written = generic_file_direct_write(iocb, from);
if (written < 0 || !iov_iter_count(from))
goto out;
- pos += written;
-
- written_buffered = fuse_perform_write(iocb, mapping, from, pos);
+ written_buffered = fuse_perform_write(iocb, mapping, from, pos = iocb->ki_pos);
if (written_buffered < 0) {
err = written_buffered;
goto out;
--
2.27.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-11-11 1:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-03 1:15 [PATCH] fuse: fix possible write position calculation error Peng Hao
2021-11-04 12:17 ` Miklos Szeredi
2021-11-05 7:43 ` Hao Peng
2021-11-10 10:40 ` Miklos Szeredi
2021-11-11 1:40 ` Hao Peng
-- strict thread matches above, loose matches on Subject: below --
2021-10-26 2:34 Peng Hao
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).