[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240805124252.nco2rblmgf6x7z4s@quack3>
Date: Mon, 5 Aug 2024 14:42:52 +0200
From: Jan Kara <jack@...e.cz>
To: Zhang Yi <yi.zhang@...weicloud.com>
Cc: Dave Chinner <david@...morbit.com>, linux-xfs@...r.kernel.org,
linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
djwong@...nel.org, hch@...radead.org, brauner@...nel.org,
jack@...e.cz, yi.zhang@...wei.com, chengzhihao1@...wei.com,
yukuai3@...wei.com
Subject: Re: [PATCH 5/6] iomap: drop unnecessary state_lock when setting ifs
uptodate bits
On Fri 02-08-24 19:13:11, Zhang Yi wrote:
> On 2024/8/2 14:29, Dave Chinner wrote:
> > On Fri, Aug 02, 2024 at 10:57:41AM +0800, Zhang Yi wrote:
> >> On 2024/8/2 8:05, Dave Chinner wrote:
> >>> On Wed, Jul 31, 2024 at 05:13:04PM +0800, Zhang Yi wrote:
> >>> Making this change also misses the elephant in the room: the
> >>> buffered write path still needs the ifs->state_lock to update the
> >>> dirty bitmap. Hence we're effectively changing the serialisation
> >>> mechanism for only one of the two ifs state bitmaps that the
> >>> buffered write path has to update.
> >>>
> >>> Indeed, we can't get rid of the ifs->state_lock from the dirty range
> >>> updates because iomap_dirty_folio() can be called without the folio
> >>> being locked through folio_mark_dirty() calling the ->dirty_folio()
> >>> aop.
> >>>
> >>
> >> Sorry, I don't understand, why folio_mark_dirty() could be called without
> >> folio lock (isn't this supposed to be a bug)? IIUC, all the file backed
> >> folios must be locked before marking dirty. Are there any exceptions or am
> >> I missing something?
> >
> > Yes: reading the code I pointed you at.
> >
> > /**
> > * folio_mark_dirty - Mark a folio as being modified.
> > * @folio: The folio.
> > *
> > * The folio may not be truncated while this function is running.
> > * Holding the folio lock is sufficient to prevent truncation, but some
> > * callers cannot acquire a sleeping lock. These callers instead hold
> > * the page table lock for a page table which contains at least one page
> > * in this folio. Truncation will block on the page table lock as it
> > * unmaps pages before removing the folio from its mapping.
> > *
> > * Return: True if the folio was newly dirtied, false if it was already dirty.
> > */
> >
> > So, yes, ->dirty_folio() can indeed be called without the folio
> > being locked and it is not a bug.
>
> Ha, right, I missed the comments of this function, it means that there are
> some special callers that hold table lock instead of folio lock, is it
> pte_alloc_map_lock?
>
> I checked all the filesystem related callers and didn't find any real
> caller that mark folio dirty without holding folio lock and that could
> affect current filesystems which are using iomap framework, it's just
> a potential possibility in the future, am I right?
There used to be quite a few places doing that. Now that I've checked all I
places was aware of got actually converted to call folio_mark_dirty() under
a folio lock (in particular all the cases happening on IO completion, folio
unmap etc.). Matthew, are you aware of any place where folio_mark_dirty()
would be called for regular file page cache (block device page cache is in a
different situation obviously) without folio lock held?
Honza
--
Jan Kara <jack@...e.com>
SUSE Labs, CR
Powered by blists - more mailing lists