| Message ID | 20231120101424.2201480-1-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | cachefiles: move kiocb_start_write() after error injection | expand |
On Mon, Nov 20, 2023 at 12:14:24PM +0200, Amir Goldstein wrote: > We want to move kiocb_start_write() into vfs_iocb_iter_write(), but > first we need to move it passed cachefiles_inject_write_error() and > prevent calling kiocb_end_write() if error was injected. > > We set the IOCB_WRITE flag after cachefiles_inject_write_error() > and use it as indication that kiocb_start_write() was called in the > cleanup/completion handler. > > Link: https://lore.kernel.org/r/CAOQ4uxihfJJRxxUhAmOwtD97Lg8PL8RgXw88rH1UfEeP8AtP+w@mail.gmail.com/ > Signed-off-by: Amir Goldstein <amir73il@gmail.com> Sorry Amir I meant to respond on Saturday but I got busy with other things. I was thinking instead, for your series, you could do something like ret = cachefiles_inject_write_error(); if (ret) { /* Start kiocb so the error handling is done below. */ kiocb_start_write(&ki->iocb); } else { ret = vfs_iocb_iter_write(file, &ki->iocb, iter); } which seems a bit cleaner than messing with the flags everywhere. Thanks, Josef
On Mon, Nov 20, 2023 at 6:56 PM Josef Bacik <josef@toxicpanda.com> wrote: > > On Mon, Nov 20, 2023 at 12:14:24PM +0200, Amir Goldstein wrote: > > We want to move kiocb_start_write() into vfs_iocb_iter_write(), but > > first we need to move it passed cachefiles_inject_write_error() and > > prevent calling kiocb_end_write() if error was injected. > > > > We set the IOCB_WRITE flag after cachefiles_inject_write_error() > > and use it as indication that kiocb_start_write() was called in the > > cleanup/completion handler. > > > > Link: https://lore.kernel.org/r/CAOQ4uxihfJJRxxUhAmOwtD97Lg8PL8RgXw88rH1UfEeP8AtP+w@mail.gmail.com/ > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > Sorry Amir I meant to respond on Saturday but I got busy with other things. > > I was thinking instead, for your series, you could do something like > > ret = cachefiles_inject_write_error(); > if (ret) { > /* Start kiocb so the error handling is done below. */ > kiocb_start_write(&ki->iocb); > } else { > ret = vfs_iocb_iter_write(file, &ki->iocb, iter); > } > > which seems a bit cleaner than messing with the flags everywhere. I think that both our options are pretty ugly ;-) I'll use whatever the maintainers of cachefiles and vfs prefer. Thanks, Amir.
Amir Goldstein <amir73il@gmail.com> wrote:
> I'll use whatever the maintainers of cachefiles and vfs prefer.
I'm working towards testing your patch. Using IOCB_WRITE as a flag like that
does seem a bit weird, though.
David
On Mon, Nov 20, 2023 at 7:05 PM Amir Goldstein <amir73il@gmail.com> wrote: > > On Mon, Nov 20, 2023 at 6:56 PM Josef Bacik <josef@toxicpanda.com> wrote: > > > > On Mon, Nov 20, 2023 at 12:14:24PM +0200, Amir Goldstein wrote: > > > We want to move kiocb_start_write() into vfs_iocb_iter_write(), but > > > first we need to move it passed cachefiles_inject_write_error() and > > > prevent calling kiocb_end_write() if error was injected. > > > > > > We set the IOCB_WRITE flag after cachefiles_inject_write_error() > > > and use it as indication that kiocb_start_write() was called in the > > > cleanup/completion handler. > > > > > > Link: https://lore.kernel.org/r/CAOQ4uxihfJJRxxUhAmOwtD97Lg8PL8RgXw88rH1UfEeP8AtP+w@mail.gmail.com/ > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > > Sorry Amir I meant to respond on Saturday but I got busy with other things. > > > > I was thinking instead, for your series, you could do something like > > > > ret = cachefiles_inject_write_error(); > > if (ret) { > > /* Start kiocb so the error handling is done below. */ > > kiocb_start_write(&ki->iocb); > > } else { > > ret = vfs_iocb_iter_write(file, &ki->iocb, iter); > > } > > > > which seems a bit cleaner than messing with the flags everywhere. > > I think that both our options are pretty ugly ;-) > > I'll use whatever the maintainers of cachefiles and vfs prefer. Looking closer, that's a self NACK. My patch moves kiocb_start_write() to after the permission hook, so calling vfs_iocb_iter_write() is no guarantee that kiocb_start_write() was called... I will send a new patch that does not change cachefiles. Thanks, Amir.
diff --git a/fs/cachefiles/io.c b/fs/cachefiles/io.c index 009d23cd435b..3e000d6ef9fc 100644 --- a/fs/cachefiles/io.c +++ b/fs/cachefiles/io.c @@ -259,7 +259,8 @@ static void cachefiles_write_complete(struct kiocb *iocb, long ret) _enter("%ld", ret); - kiocb_end_write(iocb); + if (iocb->ki_flags & IOCB_WRITE) + kiocb_end_write(iocb); if (ret < 0) trace_cachefiles_io_error(object, inode, ret, @@ -305,7 +306,6 @@ int __cachefiles_write(struct cachefiles_object *object, refcount_set(&ki->ki_refcnt, 2); ki->iocb.ki_filp = file; ki->iocb.ki_pos = start_pos; - ki->iocb.ki_flags = IOCB_DIRECT | IOCB_WRITE; ki->iocb.ki_ioprio = get_current_ioprio(); ki->object = object; ki->start = start_pos; @@ -319,16 +319,17 @@ int __cachefiles_write(struct cachefiles_object *object, ki->iocb.ki_complete = cachefiles_write_complete; atomic_long_add(ki->b_writing, &cache->b_writing); - kiocb_start_write(&ki->iocb); - get_file(ki->iocb.ki_filp); cachefiles_grab_object(object, cachefiles_obj_get_ioreq); trace_cachefiles_write(object, file_inode(file), ki->iocb.ki_pos, len); old_nofs = memalloc_nofs_save(); ret = cachefiles_inject_write_error(); - if (ret == 0) + if (ret == 0) { + ki->iocb.ki_flags = IOCB_DIRECT | IOCB_WRITE; + kiocb_start_write(&ki->iocb); ret = vfs_iocb_iter_write(file, &ki->iocb, iter); + } memalloc_nofs_restore(old_nofs); switch (ret) { case -EIOCBQUEUED:
We want to move kiocb_start_write() into vfs_iocb_iter_write(), but first we need to move it passed cachefiles_inject_write_error() and prevent calling kiocb_end_write() if error was injected. We set the IOCB_WRITE flag after cachefiles_inject_write_error() and use it as indication that kiocb_start_write() was called in the cleanup/completion handler. Link: https://lore.kernel.org/r/CAOQ4uxihfJJRxxUhAmOwtD97Lg8PL8RgXw88rH1UfEeP8AtP+w@mail.gmail.com/ Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- Hi David, Can you please review this patch so that I can add it to my series and send it to Christian? I do not have a cachefiles setup - this is only build tested. Thanks, Amir. fs/cachefiles/io.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)