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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxx>
SUSE Labs, CR




[Index of Archives]     [XFS Filesystem Development (older mail)]     [Linux Filesystem Development]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux RAID]     [Linux SCSI]


  Powered by Linux