Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
MIME-Version: 1.0
References: <71897125-e570-46ce-946a-d4729725e28f@kernel.dk>
In-Reply-To: <71897125-e570-46ce-946a-d4729725e28f@kernel.dk>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Mon, 25 Sep 2023 12:18:26 +0300
Message-ID: <CAOQ4uxj7hC5iGfJSD35xAGC97x32y9nnKk8qJa8ux6owpgT1wQ@mail.gmail.com>
Subject: Re: [PATCH] ovl: disable IOCB_DIO_CALLER_COMP
To:     Jens Axboe <axboe@kernel.dk>
Cc:     LKML <linux-kernel@vger.kernel.org>, linux-unionfs@vger.kernel.org,
        Zorro Lang <zlang@redhat.com>,
        Miklos Szeredi <miklos@szeredi.hu>,
        Christian Brauner <brauner@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Sep 25, 2023 at 9:21=E2=80=AFAM Jens Axboe <axboe@kernel.dk> wrote:
>
> overlayfs copies the kiocb flags when it sets up a new kiocb to handle
> a write, but it doesn't properly support dealing with the deferred
> caller completions of the kiocb. This means it doesn't get the final
> write completion value, and hence will complete the write with '0' as
> the result.
>
> We could support the caller completions in overlayfs, but for now let's
> just disable them in the generated write kiocb.
>
> Reported-by: Zorro Lang <zlang@redhat.com>
> Link: https://lore.kernel.org/io-uring/20230924142754.ejwsjen5pvyc32l4@de=
ll-per750-06-vm-08.rhts.eng.pek2.redhat.com/
> Fixes: 8c052fb3002e ("iomap: support IOCB_DIO_CALLER_COMP")
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>

Thanks for fixing this Jens!
If you or Christian want to send this fix to Linus, you have my ACK.

On the bright side, I am glad that you are aware of the overlayfs
"kiocb_clone" use case, which delegates/forwards the io request to
another file in another fs.

I have already posted an RFC [1] for moving this functionality to
common vfs code. My main goal was to expose it to other filesystem
(fuse), but a very desired side effect is that this functionality gets
more vfs reviewer eyes and then the chances of catching a regression
like this one during review of vfs changes hopefully increases.

As for test coverage, I need to check why my tests did not catch
this - I suspect fsx may not have been rebuilt with io_uring support,
but not sure (not near workstation atm).

If you would like to add overlayfs to your test coverage, as Zorro
explained, it is as simple as running ./check -overlay with your
existing fstests config.
./check -overlay is a relatively faster test run because many of the
tests do _notrun on overlayfs.
I don't have to tell you that io_uring code will end up running on
overlayfs in many container workloads, so it is not a niche setup.

Thanks,
Amir.

[1] https://lore.kernel.org/linux-fsdevel/20230912185408.3343163-1-amir73il=
@gmail.com/

> ---
>
> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> index 4193633c4c7a..693971d20280 100644
> --- a/fs/overlayfs/file.c
> +++ b/fs/overlayfs/file.c
> @@ -391,6 +391,12 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, st=
ruct iov_iter *iter)
>         if (!ovl_should_sync(OVL_FS(inode->i_sb)))
>                 ifl &=3D ~(IOCB_DSYNC | IOCB_SYNC);
>
> +       /*
> +        * Overlayfs doesn't support deferred completions, don't copy
> +        * this property in case it is set by the issuer.
> +        */
> +       ifl &=3D ~IOCB_DIO_CALLER_COMP;
> +
>         old_cred =3D ovl_override_creds(file_inode(file)->i_sb);
>         if (is_sync_kiocb(iocb)) {
>                 file_start_write(real.file);
>
> --
> Jens Axboe
>
>