[PATCH] cachefiles: move kiocb_start_write() after error injection

[PATCH] cachefiles: move kiocb_start_write() after error injection

[Amir Goldstein]

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(-)

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:
-- 
2.34.1

Re: [PATCH] cachefiles: move kiocb_start_write() after error injection

[Josef Bacik]

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

Re: [PATCH] cachefiles: move kiocb_start_write() after error injection

[Amir Goldstein]

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.

Re: [PATCH] cachefiles: move kiocb_start_write() after error injection

[David Howells]

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

Re: [PATCH] cachefiles: move kiocb_start_write() after error injection

[Amir Goldstein]

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.