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