Re: [PATCH v2 3/7] iomap: Remove unnecessary test from iomap_release_folio()

Ritesh Harjani
2023-06-05T21:10:08Z

Matthew Wilcox <willy@infradead.org> writes:

> On Sun, Jun 04, 2023 at 11:01:41AM -0700, Darrick J. Wong wrote:
>> On Fri, Jun 02, 2023 at 11:24:40PM +0100, Matthew Wilcox (Oracle) wrote:
>> > The check for the folio being under writeback is unnecessary; the caller
>> > has checked this and the folio is locked, so the folio cannot be under
>> > writeback at this point.
>>
>> Do we need a debug assertion here to validate that filemap_release_folio
>> has already filtered out folios unergoing writeback? The documentation
>> change in the next patch might be fine since you're the pagecache
>> maintainer.
>
> I don't think so? We don't usually include asserts in filesystems that
> the VFS is living up to its promises.
>
>> > /*
>> > - * mm accommodates an old ext3 case where clean folios might
>> > - * not have had the dirty bit cleared. Thus, it can send actual
>> > - * dirty folios to ->release_folio() via shrink_active_list();
>> > - * skip those here.
>> > + * If the folio is dirty, we refuse to release our metadata because
>> > + * it may be partially dirty (FIXME, add a test for that).
>>
>> Er... is this FIXME reflective of incomplete code?
>
> It's a note to Ritesh ;-)
>
> Once we have per-block dirty bits, if all the dirty bits are set, we
> can free the iomap_page without losing any information. But I don't
> want to introduce a dependency between this and Ritesh's work.

Sure Matthew, noted.
However this looks more like an optimization which can be done for cases
where we have all bits as dirty, than a dependency.

-ritesh