| Message ID | al4pxc2uftonry5vyunx5qblllbaakjsehrc74fbbk7pxddyv7@gn3k55eldmmp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fanotify accounting for fs/splice.c | expand |
On Tue, Jun 27, 2023 at 2:09 AM Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> wrote: > > The current behaviour caused an asymmetry where some write APIs > (write, sendfile) would notify the written-to/read-from objects, > but splice wouldn't. > > This affected userspace which used inotify, like coreutils tail -f. > typo: uses? But this comment is not very clear IMO. Please imagine that the reader is a distro maintainer or coreutils maintainer How are they supposed to react to this comment? Is this an important fix for them to backport?? How does this change actually affect tail -f? Does it fix a bug in tail -f? As far as I understand from our last conversation, the answer is that it does not fix a bug in tail -f and it won't affect tail -f at all - it would *allow* tail -f to be changed in a way that could improve some use cases. Right? improve in what way exactly. The simplest way to explain this fix perhaps would be to link to a patch to tail and explain how the kernel+tail fixes improve a use case. > Fixes: 983652c69199 ("splice: report related fsnotify events") > Link: https://lore.kernel.org/linux-fsdevel/jbyihkyk5dtaohdwjyivambb2gffyjs3dodpofafnkkunxq7bu@jngkdxx65pux/t/#u > Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> > Reviewed-by: Amir Goldstein <amir73il@gmail.com> > --- > No changes since v1 (except in the message). > > fs/splice.c | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) > > diff --git a/fs/splice.c b/fs/splice.c > index 3e06611d19ae..94fae24f9d54 100644 > --- a/fs/splice.c > +++ b/fs/splice.c > @@ -1154,7 +1154,8 @@ long do_splice(struct file *in, loff_t *off_in, struct file *out, > if ((in->f_flags | out->f_flags) & O_NONBLOCK) > flags |= SPLICE_F_NONBLOCK; > > - return splice_pipe_to_pipe(ipipe, opipe, len, flags); > + ret = splice_pipe_to_pipe(ipipe, opipe, len, flags); > + goto notify; > } > > if (ipipe) { > @@ -1182,15 +1183,12 @@ long do_splice(struct file *in, loff_t *off_in, struct file *out, > ret = do_splice_from(ipipe, out, &offset, len, flags); > file_end_write(out); > > - if (ret > 0) > - fsnotify_modify(out); > - > if (!off_out) > out->f_pos = offset; > else > *off_out = offset; > > - return ret; > + goto notify; > } > > if (opipe) { > @@ -1209,18 +1207,23 @@ long do_splice(struct file *in, loff_t *off_in, struct file *out, > > ret = splice_file_to_pipe(in, opipe, &offset, len, flags); > > - if (ret > 0) > - fsnotify_access(in); > - > if (!off_in) > in->f_pos = offset; > else > *off_in = offset; > > - return ret; > + goto notify; > } > > return -EINVAL; > + > +notify: > + if (ret > 0) { > + fsnotify_access(in); > + fsnotify_modify(out); > + } > + > + return ret; > } > Sorry I haven't noticed this in the first review, but goto is not really needed. We make the three cases if{}else if{}else if{} and return -EINVAL in the else case. It's not really that important, just a bit nicer IMO, so as you wish. Thanks, Amir.
diff --git a/fs/splice.c b/fs/splice.c index 3e06611d19ae..94fae24f9d54 100644 --- a/fs/splice.c +++ b/fs/splice.c @@ -1154,7 +1154,8 @@ long do_splice(struct file *in, loff_t *off_in, struct file *out, if ((in->f_flags | out->f_flags) & O_NONBLOCK) flags |= SPLICE_F_NONBLOCK; - return splice_pipe_to_pipe(ipipe, opipe, len, flags); + ret = splice_pipe_to_pipe(ipipe, opipe, len, flags); + goto notify; } if (ipipe) { @@ -1182,15 +1183,12 @@ long do_splice(struct file *in, loff_t *off_in, struct file *out, ret = do_splice_from(ipipe, out, &offset, len, flags); file_end_write(out); - if (ret > 0) - fsnotify_modify(out); - if (!off_out) out->f_pos = offset; else *off_out = offset; - return ret; + goto notify; } if (opipe) { @@ -1209,18 +1207,23 @@ long do_splice(struct file *in, loff_t *off_in, struct file *out, ret = splice_file_to_pipe(in, opipe, &offset, len, flags); - if (ret > 0) - fsnotify_access(in); - if (!off_in) in->f_pos = offset; else *off_in = offset; - return ret; + goto notify; } return -EINVAL; + +notify: + if (ret > 0) { + fsnotify_access(in); + fsnotify_modify(out); + } + + return ret; } static long __do_splice(struct file *in, loff_t __user *off_in,
The current behaviour caused an asymmetry where some write APIs (write, sendfile) would notify the written-to/read-from objects, but splice wouldn't. This affected userspace which used inotify, like coreutils tail -f. Fixes: 983652c69199 ("splice: report related fsnotify events") Link: https://lore.kernel.org/linux-fsdevel/jbyihkyk5dtaohdwjyivambb2gffyjs3dodpofafnkkunxq7bu@jngkdxx65pux/t/#u Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz> Reviewed-by: Amir Goldstein <amir73il@gmail.com> --- No changes since v1 (except in the message). fs/splice.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)