[PATCH] netfs: Fix setting of BDP_ASYNC from iocb flags

[PATCH] netfs: Fix setting of BDP_ASYNC from iocb flags

[David Howells]

Fix netfs_perform_write() to set BDP_ASYNC if IOCB_NOWAIT is set rather
than if IOCB_SYNC is not set.  It reflects asynchronicity in the sense of
not waiting rather than synchronicity in the sense of not returning until
the op is complete.

Without this, generic/590 fails on cifs in strict caching mode with a
complaint that one of the writes fails with EAGAIN.  The test can be
distilled down to:

        mount -t cifs /my/share /mnt -ostuff
        xfs_io -i -c 'falloc 0 8191M -c fsync -f /mnt/file
        xfs_io -i -c 'pwrite -b 1M -W 0 8191M' /mnt/file

Fixes: c38f4e96e605 ("netfs: Provide func to copy data to pagecache for buffered write")
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Jeff Layton <jlayton@kernel.org>
cc: Enzo Matsumiya <ematsumiya@suse.de>
cc: Jens Axboe <axboe@kernel.dk>
cc: Matthew Wilcox <willy@infradead.org>
cc: netfs@lists.linux.dev
cc: v9fs@lists.linux.dev
cc: linux-afs@lists.infradead.org
cc: linux-cifs@vger.kernel.org
cc: linux-fsdevel@vger.kernel.org
---
 fs/netfs/buffered_write.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/netfs/buffered_write.c b/fs/netfs/buffered_write.c
index 1121601536d1..07bc1fd43530 100644
--- a/fs/netfs/buffered_write.c
+++ b/fs/netfs/buffered_write.c
@@ -181,7 +181,7 @@ ssize_t netfs_perform_write(struct kiocb *iocb, struct iov_iter *iter,
 	struct folio *folio, *writethrough = NULL;
 	enum netfs_how_to_modify howto;
 	enum netfs_folio_trace trace;
-	unsigned int bdp_flags = (iocb->ki_flags & IOCB_SYNC) ? 0: BDP_ASYNC;
+	unsigned int bdp_flags = (iocb->ki_flags & IOCB_NOWAIT) ? BDP_ASYNC : 0;
 	ssize_t written = 0, ret, ret2;
 	loff_t i_size, pos = iocb->ki_pos, from, to;
 	size_t max_chunk = PAGE_SIZE << MAX_PAGECACHE_ORDER;

Re: [PATCH] netfs: Fix setting of BDP_ASYNC from iocb flags

[Jens Axboe]

On 5/21/24 9:49 AM, David Howells wrote:
> Fix netfs_perform_write() to set BDP_ASYNC if IOCB_NOWAIT is set rather
> than if IOCB_SYNC is not set.  It reflects asynchronicity in the sense of
> not waiting rather than synchronicity in the sense of not returning until
> the op is complete.
> 
> Without this, generic/590 fails on cifs in strict caching mode with a
> complaint that one of the writes fails with EAGAIN.  The test can be
> distilled down to:
> 
>         mount -t cifs /my/share /mnt -ostuff
>         xfs_io -i -c 'falloc 0 8191M -c fsync -f /mnt/file
>         xfs_io -i -c 'pwrite -b 1M -W 0 8191M' /mnt/file

Looks good to me:

Reviewed-by: Jens Axboe <axboe@kernel.dk>

However, I'll note that BDP_ASYNC is horribly named, it should be
BDP_NOWAIT instead. But that's a separate thing, fix looks correct
as-is.

-- 
Jens Axboe

Re: [PATCH] netfs: Fix setting of BDP_ASYNC from iocb flags

[David Howells]

Jens Axboe <axboe@kernel.dk> wrote:

> However, I'll note that BDP_ASYNC is horribly named, it should be
> BDP_NOWAIT instead. But that's a separate thing, fix looks correct
> as-is.

I thought IOCB_NOWAIT was related to RWF_NOWAIT, but apparently not from the
code.

David

Re: [PATCH] netfs: Fix setting of BDP_ASYNC from iocb flags

[Jens Axboe]

On 5/21/24 9:54 AM, David Howells wrote:
> Jens Axboe <axboe@kernel.dk> wrote:
> 
>> However, I'll note that BDP_ASYNC is horribly named, it should be
>> BDP_NOWAIT instead. But that's a separate thing, fix looks correct
>> as-is.
> 
> I thought IOCB_NOWAIT was related to RWF_NOWAIT, but apparently not from the
> code.

It is, something submitted with RWF_NOWAIT should have IOCB_NOWAIT set.
But RWF_NOWAIT isn't the sole user of IOCB_NOWAIT, and no assumptions
should be made about whether something is sync or async based on whether
or not RWF_NOWAIT is set. Those aren't related other than _some_ proper
async IO will have IOCB_NOWAIT set, and others will not.

-- 
Jens Axboe

Re: [PATCH] netfs: Fix setting of BDP_ASYNC from iocb flags

[David Howells]

Jens Axboe <axboe@kernel.dk> wrote:

> On 5/21/24 9:54 AM, David Howells wrote:
> > Jens Axboe <axboe@kernel.dk> wrote:
> > 
> >> However, I'll note that BDP_ASYNC is horribly named, it should be
> >> BDP_NOWAIT instead. But that's a separate thing, fix looks correct
> >> as-is.
> > 
> > I thought IOCB_NOWAIT was related to RWF_NOWAIT, but apparently not from the
> > code.
> 
> It is, something submitted with RWF_NOWAIT should have IOCB_NOWAIT set.
> But RWF_NOWAIT isn't the sole user of IOCB_NOWAIT, and no assumptions
> should be made about whether something is sync or async based on whether
> or not RWF_NOWAIT is set. Those aren't related other than _some_ proper
> async IO will have IOCB_NOWAIT set, and others will not.

Are you sure?  RWF_NOWAIT seems to set IOCB_NOIO.

David

Re: [PATCH] netfs: Fix setting of BDP_ASYNC from iocb flags

[Jens Axboe]

On 5/21/24 2:05 PM, David Howells wrote:
> Jens Axboe <axboe@kernel.dk> wrote:
> 
>> On 5/21/24 9:54 AM, David Howells wrote:
>>> Jens Axboe <axboe@kernel.dk> wrote:
>>>
>>>> However, I'll note that BDP_ASYNC is horribly named, it should be
>>>> BDP_NOWAIT instead. But that's a separate thing, fix looks correct
>>>> as-is.
>>>
>>> I thought IOCB_NOWAIT was related to RWF_NOWAIT, but apparently not from the
>>> code.
>>
>> It is, something submitted with RWF_NOWAIT should have IOCB_NOWAIT set.
>> But RWF_NOWAIT isn't the sole user of IOCB_NOWAIT, and no assumptions
>> should be made about whether something is sync or async based on whether
>> or not RWF_NOWAIT is set. Those aren't related other than _some_ proper
>> async IO will have IOCB_NOWAIT set, and others will not.
> 
> Are you sure?  RWF_NOWAIT seems to set IOCB_NOIO.

As it should, no-wait should imply not blocking on other IO. This is
completely orthogonal to whether or not it's async or sync IO.

I have a distinct feeling we're talking past each other :-)

-- 
Jens Axboe

Re: [PATCH] netfs: Fix setting of BDP_ASYNC from iocb flags

[Christian Brauner]

On Tue, 21 May 2024 16:49:46 +0100, David Howells wrote:
> Fix netfs_perform_write() to set BDP_ASYNC if IOCB_NOWAIT is set rather
> than if IOCB_SYNC is not set.  It reflects asynchronicity in the sense of
> not waiting rather than synchronicity in the sense of not returning until
> the op is complete.
> 
> Without this, generic/590 fails on cifs in strict caching mode with a
> complaint that one of the writes fails with EAGAIN.  The test can be
> distilled down to:
> 
> [...]

Applied to the vfs.fixes branch of the vfs/vfs.git tree.
Patches in the vfs.fixes branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.fixes

[1/1] netfs: Fix setting of BDP_ASYNC from iocb flags
      https://git.kernel.org/vfs/vfs/c/33c9d7477ef1

Re: [PATCH] netfs: Fix setting of BDP_ASYNC from iocb flags

[Steve French]

You can add my Tested-by if you would like

On Wed, May 22, 2024 at 2:14 AM Christian Brauner <brauner@kernel.org> wrote:
>
> On Tue, 21 May 2024 16:49:46 +0100, David Howells wrote:
> > Fix netfs_perform_write() to set BDP_ASYNC if IOCB_NOWAIT is set rather
> > than if IOCB_SYNC is not set.  It reflects asynchronicity in the sense of
> > not waiting rather than synchronicity in the sense of not returning until
> > the op is complete.
> >
> > Without this, generic/590 fails on cifs in strict caching mode with a
> > complaint that one of the writes fails with EAGAIN.  The test can be
> > distilled down to:
> >
> > [...]
>
> Applied to the vfs.fixes branch of the vfs/vfs.git tree.
> Patches in the vfs.fixes branch should appear in linux-next soon.
>
> Please report any outstanding bugs that were missed during review in a
> new review to the original patch series allowing us to drop it.
>
> It's encouraged to provide Acked-bys and Reviewed-bys even though the
> patch has now been applied. If possible patch trailers will be updated.
>
> Note that commit hashes shown below are subject to change due to rebase,
> trailer updates or similar. If in doubt, please check the listed branch.
>
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
> branch: vfs.fixes
>
> [1/1] netfs: Fix setting of BDP_ASYNC from iocb flags
>       https://git.kernel.org/vfs/vfs/c/33c9d7477ef1
>


-- 
Thanks,

Steve