Re: [PATCH 5/6] iomap: drop unnecessary state_lock when setting ifs uptodate bits

[Jan Kara <jack@suse.cz>]

Actually add Matthew to CC ;)

On Mon 05-08-24 14:42:52, Jan Kara wrote:
> 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
> places I 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@suse.com>
> SUSE Labs, CR
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR