Re: [PATCH 12/16] ext4: add RWF_UNCACHED write support

Jens Axboe
2024-11-12T17:13:15Z

On 11/12/24 9:36 AM, Brian Foster wrote:
> On Mon, Nov 11, 2024 at 04:37:39PM -0700, Jens Axboe wrote:
>> IOCB_UNCACHED IO needs to prune writeback regions on IO completion,
>> and hence need the worker punt that ext4 also does for unwritten
>> extents. Add an io_end flag to manage that.
>>
>> If foliop is set to foliop_uncached in ext4_write_begin(), then set
>> FGP_UNCACHED so that __filemap_get_folio() will mark newly created
>> folios as uncached. That in turn will make writeback completion drop
>> these ranges from the page cache.
>>
>> Now that ext4 supports both uncached reads and writes, add the fop_flag
>> FOP_UNCACHED to enable it.
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>> ---
>> fs/ext4/ext4.h | 1 +
>> fs/ext4/file.c | 2 +-
>> fs/ext4/inline.c | 7 ++++++-
>> fs/ext4/inode.c | 18 ++++++++++++++++--
>> fs/ext4/page-io.c | 28 ++++++++++++++++------------
>> 5 files changed, 40 insertions(+), 16 deletions(-)
>>
> ...
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index 54bdd4884fe6..afae3ab64c9e 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
>> @@ -1138,6 +1138,7 @@ static int ext4_write_begin(struct file *file, struct address_space *mapping,
>> int ret, needed_blocks;
>> handle_t *handle;
>> int retries = 0;
>> + fgf_t fgp_flags;
>> struct folio *folio;
>> pgoff_t index;
>> unsigned from, to;
>> @@ -1164,6 +1165,15 @@ static int ext4_write_begin(struct file *file, struct address_space *mapping,
>> return 0;
>> }
>>
>> + /*
>> + * Set FGP_WRITEBEGIN, and FGP_UNCACHED if foliop contains
>> + * foliop_uncached. That's how generic_perform_write() informs us
>> + * that this is an uncached write.
>> + */
>> + fgp_flags = FGP_WRITEBEGIN;
>> + if (*foliop == foliop_uncached)
>> + fgp_flags |= FGP_UNCACHED;
>> +
>> /*
>> * __filemap_get_folio() can take a long time if the
>> * system is thrashing due to memory pressure, or if the folio
>> @@ -1172,7 +1182,7 @@ static int ext4_write_begin(struct file *file, struct address_space *mapping,
>> * the folio (if needed) without using GFP_NOFS.
>> */
>> retry_grab:
>> - folio = __filemap_get_folio(mapping, index, FGP_WRITEBEGIN,
>> + folio = __filemap_get_folio(mapping, index, fgp_flags,
>> mapping_gfp_mask(mapping));
>> if (IS_ERR(folio))
>> return PTR_ERR(folio);
>
> JFYI, I notice that ext4 cycles the folio lock here in this path and
> thus follows up with a couple checks presumably to accommodate that. One
> is whether i_mapping has changed, which I assume means uncached state
> would have been handled/cleared externally somewhere..? I.e., if an
> uncached folio is somehow truncated/freed without ever having been
> written back?
>
> The next is a folio_wait_stable() call "in case writeback began ..."
> It's not immediately clear to me if that is possible here, but taking
> that at face value, is it an issue if we were to create an uncached
> folio, drop the folio lock, then have some other task dirty and
> writeback the folio (due to a sync write or something), then have
> writeback completion invalidate the folio before we relock it here?

I don't either of those are an issue. The UNCACHED flag will only be set
on a newly created folio, it does not get inherited for folios that
already exist.

--
Jens Axboe