Hi,
I came across this patch in a news posting:
https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=d618126911829523e35a61f4a5a4ad159b1b2c8d
which has me a bit worried. As far as I can tell, all that patch does is
set FMODE_BUF_WASYNC, and then just hope that the lower layers handle
the rest?
What happens if iocb->ki_flags & IOCB_NOWAIT is true, and now we do:
generic_perform_write(iocb, from)
...
->write_begin() <- does this block?
...
->write_end() <- or this one?
...
balance_dirty_pages_ratelimited() <- this one surely does...
If you look just one level down the latter to
balance_dirty_pages_ratelimited_flags(), you'll even see the 'flags'
argument documented there.
This looks pretty haphazard and cannot possibly work as-is, so please
get this reverted until f2fs is converted to iomap, or IOCB_NOWAIT is
handled by generic_perform_write() and below.
--
Jens Axboe
On 06/19, Jens Axboe wrote:
> Hi,
>
> I came across this patch in a news posting:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=d618126911829523e35a61f4a5a4ad159b1b2c8d
>
> which has me a bit worried. As far as I can tell, all that patch does is
> set FMODE_BUF_WASYNC, and then just hope that the lower layers handle
> the rest?
>
> What happens if iocb->ki_flags & IOCB_NOWAIT is true, and now we do:
>
> generic_perform_write(iocb, from)
> ...
> ->write_begin() <- does this block?
> ...
> ->write_end() <- or this one?
> ...
> balance_dirty_pages_ratelimited() <- this one surely does...
>
> If you look just one level down the latter to
> balance_dirty_pages_ratelimited_flags(), you'll even see the 'flags'
> argument documented there.
>
> This looks pretty haphazard and cannot possibly work as-is, so please
> get this reverted until f2fs is converted to iomap, or IOCB_NOWAIT is
> handled by generic_perform_write() and below.
Thank you for pointing that out. It seems I haven't reviewed it carefully.
Hence I removed it from -next, and hope to have some time to convert iomap
soon.
Thanks,
>
> --
> Jens Axboe
On 6/20/23 12:16?AM, Jaegeuk Kim wrote:
> On 06/19, Jens Axboe wrote:
>> Hi,
>>
>> I came across this patch in a news posting:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=d618126911829523e35a61f4a5a4ad159b1b2c8d
>>
>> which has me a bit worried. As far as I can tell, all that patch does is
>> set FMODE_BUF_WASYNC, and then just hope that the lower layers handle
>> the rest?
>>
>> What happens if iocb->ki_flags & IOCB_NOWAIT is true, and now we do:
>>
>> generic_perform_write(iocb, from)
>> ...
>> ->write_begin() <- does this block?
>> ...
>> ->write_end() <- or this one?
>> ...
>> balance_dirty_pages_ratelimited() <- this one surely does...
>>
>> If you look just one level down the latter to
>> balance_dirty_pages_ratelimited_flags(), you'll even see the 'flags'
>> argument documented there.
>>
>> This looks pretty haphazard and cannot possibly work as-is, so please
>> get this reverted until f2fs is converted to iomap, or IOCB_NOWAIT is
>> handled by generic_perform_write() and below.
>
> Thank you for pointing that out. It seems I haven't reviewed it carefully.
> Hence I removed it from -next, and hope to have some time to convert iomap
> soon.
Thanks - would be great to get FMODE_BUF_WASYNC enabled obviously, just
needs a bit more work to get there.
--
Jens Axboe
+cc [email protected]
On 2023/6/20 4:43, Jens Axboe wrote:
> Hi,
>
> I came across this patch in a news posting:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=d618126911829523e35a61f4a5a4ad159b1b2c8d
>
> which has me a bit worried. As far as I can tell, all that patch does is
> set FMODE_BUF_WASYNC, and then just hope that the lower layers handle
> the rest?
>
> What happens if iocb->ki_flags & IOCB_NOWAIT is true, and now we do:
>
> generic_perform_write(iocb, from)
> ...
> ->write_begin() <- does this block?
Most file systems have not been changed to the iomap method,
and the generic_perform_write method is still used. So it seems
reasonable to make IOCB_NOWAIT be handled correctly by the
generic_perform_write function. This means that we need to modify
several places mentioned by Jens in the generic_perform_write function,
and need to pass AOP_FLAG_xxx or iocb into write_begin.
I noticed that Matthew Wilcox removed the flags parameter in write_begin
in a previous commit, maybe we can add it back?
Thx
> ...
> ->write_end() <- or this one?
> ...
> balance_dirty_pages_ratelimited() <- this one surely does...
>
> If you look just one level down the latter to
> balance_dirty_pages_ratelimited_flags(), you'll even see the 'flags'
> argument documented there.
>
> This looks pretty haphazard and cannot possibly work as-is, so please
> get this reverted until f2fs is converted to iomap, or IOCB_NOWAIT is
> handled by generic_perform_write() and below.
>
On Mon, Jun 26, 2023 at 03:31:57PM +0800, Yangtao Li wrote:
> +cc [email protected]
>
>
> On 2023/6/20 4:43, Jens Axboe wrote:
>
> > Hi,
> >
> > I came across this patch in a news posting:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git/commit/?h=dev&id=d618126911829523e35a61f4a5a4ad159b1b2c8d
> >
> > which has me a bit worried. As far as I can tell, all that patch does is
> > set FMODE_BUF_WASYNC, and then just hope that the lower layers handle
> > the rest?
> >
> > What happens if iocb->ki_flags & IOCB_NOWAIT is true, and now we do:
> >
> > generic_perform_write(iocb, from)
> > ...
> > ->write_begin() <- does this block?
>
> Most file systems have not been changed to the iomap method,
That's what needs to be fixed.
> and the generic_perform_write method is still used. So it seems
>
> ?reasonable to make IOCB_NOWAIT be handled correctly by the
>
> ?generic_perform_write function. This means that we need to modify
No. Use the modern infrastructure.
> ?several places mentioned by Jens in the generic_perform_write function,
>
> ?and need to pass AOP_FLAG_xxx or iocb into write_begin.
Definitely not.
>
> I noticed that Matthew Wilcox removed the flags parameter in write_begin
>
> in a previous commit, maybe we can add it back?
No.