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
