diff for duplicates of <c66ca795-da93-437c-bb11-718801f8114a@kernel.dk> diff --git a/a/1.txt b/N1/1.txt index dc6a452..c48e021 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -1,64 +1,33 @@ +From: Jens Axboe <axboe@kernel.dk> + On 5/27/24 4:09 AM, Liu Wei wrote: -> I am a newer, thanks for the reminder. +> > I am a newer, thanks for the reminder. +> > +> >> +> >> I don't think WB_SYNC_NONE tells it not to block, it just says not to +> >> wait for it... So this won't work as-is. +> > +> > Yes, but I think an asynchronous writex-back is better than simply +> > return EAGAIN. By using __filemap_fdatawrite_range to trigger a +> > writeback, subsequent retries may have a higher chance of success. > ->> ->>>> when we issuing AIO with direct I/O and IOCB_NOWAIT on a block device, the ->>>> process context will not be blocked. ->>>> ->>>> However, if the device already has page cache in memory, EAGAIN will be ->>>> returned. And even when trying to reissue the AIO with direct I/O and ->>>> IOCB_NOWAIT again, we consistently receive EAGAIN. ->> ->> -EAGAIN doesn't mean "just try again and it'll work". ->> ->>>> Maybe a better way to deal with it: filemap_fdatawrite_range dirty pages ->>>> with WB_SYNC_NONE flag, and invalidate_mapping_pages unmapped pages at ->>>> the same time. ->>>> ->>>> Signed-off-by: Liu Wei <liuwei09@cestc.cn> ->>>> --- ->>>> mm/filemap.c | 9 ++++++++- ->>>> 1 file changed, 8 insertions(+), 1 deletion(-) ->>>> ->>>> diff --git a/mm/filemap.c b/mm/filemap.c ->>>> index 30de18c4fd28..1852a00caf31 100644 ->>>> --- a/mm/filemap.c ->>>> +++ b/mm/filemap.c ->>>> @@ -2697,8 +2697,15 @@ int kiocb_invalidate_pages(struct kiocb *iocb, size_t count) ->>>> ->>>> if (iocb->ki_flags & IOCB_NOWAIT) { ->>>> /* we could block if there are any pages in the range */ ->>>> - if (filemap_range_has_page(mapping, pos, end)) ->>>> + if (filemap_range_has_page(mapping, pos, end)) { ->>>> + if (mapping_needs_writeback(mapping)) { ->>>> + __filemap_fdatawrite_range(mapping, ->>>> + pos, end, WB_SYNC_NONE); ->>>> + } ->> ->> I don't think WB_SYNC_NONE tells it not to block, it just says not to ->> wait for it... So this won't work as-is. +> And what's the application supposed to do, just hammer on the same +> IOCB_NOWAIT submission until it then succeeds? The only way this can +> reasonably work for that would be if yo can do: > -> Yes, but I think an asynchronous writex-back is better than simply -> return EAGAIN. By using __filemap_fdatawrite_range to trigger a -> writeback, subsequent retries may have a higher chance of success. - -And what's the application supposed to do, just hammer on the same -IOCB_NOWAIT submission until it then succeeds? The only way this can -reasonably work for that would be if yo can do: - -1) Issue IOCB_NOWAIT IO -2) Get -EAGAIN -3) Sync kick off writeback, wait for it to be done -4) Issue IOCB_NOWAIT IO again -5) Success - -If you just kick it off, then you'd repeat steps 1..2 ad nauseam until -it works out, not tenable. - -And this doesn't even include the other point I mentioned, which is -__filemap_fdatawrite_range() IO issue blocking in the first place. - -So no, NAK on this patch. +> 1) Issue IOCB_NOWAIT IO +> 2) Get -EAGAIN +> 3) Sync kick off writeback, wait for it to be done +> 4) Issue IOCB_NOWAIT IO again +> 5) Success +> +> If you just kick it off, then you'd repeat steps 1..2 ad nauseam until +> it works out, not tenable. +> +> And this doesn't even include the other point I mentioned, which is +> __filemap_fdatawrite_range() IO issue blocking in the first place. +> +> So no, NAK on this patch. +> --- -Jens Axboe +I know, thanks for your patient explanation. diff --git a/a/content_digest b/N1/content_digest index ba9a629..432a8a9 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -1,81 +1,51 @@ "ref\0024b9a30-ad3b-4063-b5c8-e6c948ad6b2e@kernel.dk\0" "ref\020240527100908.49913-1-liuwei09@cestc.cn\0" - "From\0Jens Axboe <axboe@kernel.dk>\0" + "From\0Liu Wei <liuwei09@cestc.cn>\0" "Subject\0Re: [PATCH] mm/filemap: invalidating pages is still necessary when io with IOCB_NOWAIT\0" - "Date\0Mon, 27 May 2024 09:36:24 -0600\0" - "To\0Liu Wei <liuwei09@cestc.cn>\0" + "Date\0Thu, 30 May 2024 10:33:04 +0800\0" + "To\0liuwei09@cestc.cn\0" "Cc\0akpm@linux-foundation.org" hch@lst.de jack@suse.cz linux-fsdevel@vger.kernel.org linux-kernel@vger.kernel.org rgoldwyn@suse.com - " willy@infradead.org\0" + willy@infradead.org + " Jens Axboe <axboe@kernel.dk>\0" "\00:1\0" "b\0" + "From: Jens Axboe <axboe@kernel.dk>\n" + "\n" "On 5/27/24 4:09 AM, Liu Wei wrote:\n" - "> I am a newer, thanks for the reminder.\n" + "> > I am a newer, thanks for the reminder.\n" + "> > \n" + "> >>\n" + "> >> I don't think WB_SYNC_NONE tells it not to block, it just says not to\n" + "> >> wait for it... So this won't work as-is.\n" + "> > \n" + "> > Yes, but I think an asynchronous writex-back is better than simply\n" + "> > return EAGAIN. By using __filemap_fdatawrite_range to trigger a\n" + "> > writeback, subsequent retries may have a higher chance of success. \n" "> \n" - ">>\n" - ">>>> when we issuing AIO with direct I/O and IOCB_NOWAIT on a block device, the\n" - ">>>> process context will not be blocked.\n" - ">>>>\n" - ">>>> However, if the device already has page cache in memory, EAGAIN will be\n" - ">>>> returned. And even when trying to reissue the AIO with direct I/O and\n" - ">>>> IOCB_NOWAIT again, we consistently receive EAGAIN.\n" - ">>\n" - ">> -EAGAIN doesn't mean \"just try again and it'll work\".\n" - ">>\n" - ">>>> Maybe a better way to deal with it: filemap_fdatawrite_range dirty pages\n" - ">>>> with WB_SYNC_NONE flag, and invalidate_mapping_pages unmapped pages at\n" - ">>>> the same time.\n" - ">>>>\n" - ">>>> Signed-off-by: Liu Wei <liuwei09@cestc.cn>\n" - ">>>> ---\n" - ">>>> mm/filemap.c | 9 ++++++++-\n" - ">>>> 1 file changed, 8 insertions(+), 1 deletion(-)\n" - ">>>>\n" - ">>>> diff --git a/mm/filemap.c b/mm/filemap.c\n" - ">>>> index 30de18c4fd28..1852a00caf31 100644\n" - ">>>> --- a/mm/filemap.c\n" - ">>>> +++ b/mm/filemap.c\n" - ">>>> @@ -2697,8 +2697,15 @@ int kiocb_invalidate_pages(struct kiocb *iocb, size_t count)\n" - ">>>> \n" - ">>>> \tif (iocb->ki_flags & IOCB_NOWAIT) {\n" - ">>>> \t\t/* we could block if there are any pages in the range */\n" - ">>>> -\t\tif (filemap_range_has_page(mapping, pos, end))\n" - ">>>> +\t\tif (filemap_range_has_page(mapping, pos, end)) {\n" - ">>>> +\t\t\tif (mapping_needs_writeback(mapping)) {\n" - ">>>> +\t\t\t\t__filemap_fdatawrite_range(mapping,\n" - ">>>> +\t\t\t\t\t\tpos, end, WB_SYNC_NONE);\n" - ">>>> +\t\t\t}\n" - ">>\n" - ">> I don't think WB_SYNC_NONE tells it not to block, it just says not to\n" - ">> wait for it... So this won't work as-is.\n" + "> And what's the application supposed to do, just hammer on the same\n" + "> IOCB_NOWAIT submission until it then succeeds? The only way this can\n" + "> reasonably work for that would be if yo can do:\n" "> \n" - "> Yes, but I think an asynchronous writex-back is better than simply\n" - "> return EAGAIN. By using __filemap_fdatawrite_range to trigger a\n" - "> writeback, subsequent retries may have a higher chance of success. \n" - "\n" - "And what's the application supposed to do, just hammer on the same\n" - "IOCB_NOWAIT submission until it then succeeds? The only way this can\n" - "reasonably work for that would be if yo can do:\n" - "\n" - "1) Issue IOCB_NOWAIT IO\n" - "2) Get -EAGAIN\n" - "3) Sync kick off writeback, wait for it to be done\n" - "4) Issue IOCB_NOWAIT IO again\n" - "5) Success\n" - "\n" - "If you just kick it off, then you'd repeat steps 1..2 ad nauseam until\n" - "it works out, not tenable.\n" - "\n" - "And this doesn't even include the other point I mentioned, which is\n" - "__filemap_fdatawrite_range() IO issue blocking in the first place.\n" - "\n" - "So no, NAK on this patch.\n" + "> 1) Issue IOCB_NOWAIT IO\n" + "> 2) Get -EAGAIN\n" + "> 3) Sync kick off writeback, wait for it to be done\n" + "> 4) Issue IOCB_NOWAIT IO again\n" + "> 5) Success\n" + "> \n" + "> If you just kick it off, then you'd repeat steps 1..2 ad nauseam until\n" + "> it works out, not tenable.\n" + "> \n" + "> And this doesn't even include the other point I mentioned, which is\n" + "> __filemap_fdatawrite_range() IO issue blocking in the first place.\n" + "> \n" + "> So no, NAK on this patch.\n" + ">\n" "\n" - "-- \n" - Jens Axboe + I know, thanks for your patient explanation. -a011a9c8bcd140b5b1f8bade26351e735efacca731bd6dcf716c843c636c87f6 +f1454dc503582680eb594db0b78321c544fbac5ac7a4a2aedec251e27d058c85
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox