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 <[email protected]>
cc: Jeff Layton <[email protected]>
cc: Enzo Matsumiya <[email protected]>
cc: Jens Axboe <[email protected]>
cc: Matthew Wilcox <[email protected]>
cc: [email protected]
cc: [email protected]
cc: [email protected]
cc: [email protected]
cc: [email protected]
---
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;
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 <[email protected]>
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
Jens Axboe <[email protected]> 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
On 5/21/24 9:54 AM, David Howells wrote:
> Jens Axboe <[email protected]> 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
Jens Axboe <[email protected]> wrote:
> On 5/21/24 9:54 AM, David Howells wrote:
> > Jens Axboe <[email protected]> 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
On 5/21/24 2:05 PM, David Howells wrote:
> Jens Axboe <[email protected]> wrote:
>
>> On 5/21/24 9:54 AM, David Howells wrote:
>>> Jens Axboe <[email protected]> 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
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
You can add my Tested-by if you would like
On Wed, May 22, 2024 at 2:14 AM Christian Brauner <[email protected]> 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