linux-kernel - Re: [PATCH] fs: clean up usage of noop_dirty

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZONWka8NpDVGzI8h@casper.infradead.org>
Date:   Mon, 21 Aug 2023 13:20:33 +0100
From:   Matthew Wilcox <willy@...radead.org>
To:     Jan Kara <jack@...e.cz>
Cc:     Xueshi Hu <xueshi.hu@...rtx.com>, dan.j.williams@...el.com,
        vishal.l.verma@...el.com, dave.jiang@...el.com,
        jayalk@...works.biz, daniel@...ll.ch, deller@....de,
        bcrl@...ck.org, viro@...iv.linux.org.uk, brauner@...nel.org,
        jack@...e.com, tytso@....edu, adilger.kernel@...ger.ca,
        miklos@...redi.hu, mike.kravetz@...cle.com, muchun.song@...ux.dev,
        djwong@...nel.org, akpm@...ux-foundation.org, hughd@...gle.com,
        nvdimm@...ts.linux.dev, linux-cxl@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-fbdev@...r.kernel.org,
        dri-devel@...ts.freedesktop.org, linux-aio@...ck.org,
        linux-fsdevel@...r.kernel.org, linux-ext4@...r.kernel.org,
        linux-mm@...ck.org, linux-xfs@...r.kernel.org
Subject: Re: [PATCH] fs: clean up usage of noop_dirty_folio

On Mon, Aug 21, 2023 at 01:16:43PM +0200, Jan Kara wrote:
> On Sat 19-08-23 20:42:25, Xueshi Hu wrote:
> > In folio_mark_dirty(), it will automatically fallback to
> > noop_dirty_folio() if a_ops->dirty_folio is not registered.
> > 
> > As anon_aops, dev_dax_aops and fb_deferred_io_aops becames empty, remove
> > them too.
> > 
> > Signed-off-by: Xueshi Hu <xueshi.hu@...rtx.com>
> 
> Yeah, looks sensible to me but for some callbacks we are oscilating between
> all users having to provide some callback and providing some default
> behavior for NULL callback. I don't have a strong opinion either way so
> feel free to add:
> 
> Reviewed-by: Jan Kara <jack@...e.cz>
> 
> But I guess let's see what Matthew thinks about this and what plans he has
> so that we don't switch back again in the near future. Matthew?

I was hoping Christoph would weigh in ;-)  I don't have a strong
feeling here, but it seems to me that a NULL ->dirty_folio() should mean
"do the noop thing" rather than "do the buffer_head thing" or "do the
filemap thing".  In 0af573780b0b, the buffer_head default was removed.
I think enough time has passed that we're OK to change what a NULL
->dirty_folio means (plus we also changed the name of ->set_page_dirty()
to ->dirty_folio())

So Ack to the concept.  One minor change I'd request:

-bool noop_dirty_folio(struct address_space *mapping, struct folio *folio)
+static bool noop_dirty_folio(struct address_space *mapping, struct folio *folio)
 {
 	if (!folio_test_dirty(folio))
 		return !folio_test_set_dirty(folio);
 	return false;
 }
-EXPORT_SYMBOL(noop_dirty_folio);

Please inline this into folio_mark_dirty() instead of calling it.