On Fri, Apr 26, 2024 at 10:09:22PM +0530, Ritesh Harjani wrote: > Zhang Yi <yi.zhang@xxxxxxxxxxxxxxx> writes: > > > On 2024/4/26 20:57, Ritesh Harjani (IBM) wrote: > >> Ritesh Harjani (IBM) <ritesh.list@xxxxxxxxx> writes: > >> > >>> Zhang Yi <yi.zhang@xxxxxxxxxxxxxxx> writes: > >>> > >>>> From: Zhang Yi <yi.zhang@xxxxxxxxxx> > >>>> > >>>> Now we lookup extent status entry without holding the i_data_sem before > >>>> inserting delalloc block, it works fine in buffered write path and > >>>> because it holds i_rwsem and folio lock, and the mmap path holds folio > >>>> lock, so the found extent locklessly couldn't be modified concurrently. > >>>> But it could be raced by fallocate since it allocate block whitout > >>>> holding i_rwsem and folio lock. > >>>> > >>>> ext4_page_mkwrite() ext4_fallocate() > >>>> block_page_mkwrite() > >>>> ext4_da_map_blocks() > >>>> //find hole in extent status tree > >>>> ext4_alloc_file_blocks() > >>>> ext4_map_blocks() > >>>> //allocate block and unwritten extent > >>>> ext4_insert_delayed_block() > >>>> ext4_da_reserve_space() > >>>> //reserve one more block > >>>> ext4_es_insert_delayed_block() > >>>> //drop unwritten extent and add delayed extent by mistake > >>>> > >>>> Then, the delalloc extent is wrong until writeback, the one more > >>>> reserved block can't be release any more and trigger below warning: > >>>> > >>>> EXT4-fs (pmem2): Inode 13 (00000000bbbd4d23): i_reserved_data_blocks(1) not cleared! > >>>> > >>>> Hold i_data_sem in write mode directly can fix the problem, but it's > >>>> expansive, we should keep the lockless check and check the extent again > >>>> once we need to add an new delalloc block. > >>> > >>> Hi Zhang, > >>> > >>> It's a nice finding. I was wondering if this was caught in any of the > >>> xfstests? > >>> > > > > Hi, Ritesh > > > > I caught this issue when I tested my iomap series in generic/344 and > > generic/346. It's easy to reproduce because the iomap's buffered write path > > doesn't hold folio lock while inserting delalloc blocks, so it could be raced > > by the mmap page fault path. But the buffer_head's buffered write path can't > > trigger this problem, > > ya right! That's the difference between how ->map_blocks() is called > between buffer_head v/s iomap path. In iomap the ->map_blocks() call > happens first to map a large extent and then it iterate over all the > locked folios covering the mapped extent for doing writes. Yes - a fundamental property of the iomap is that it is cached filesystem state that isn't protected by locks in any way. It can become stale if a concurrent operation modifies the extent map whilst the write operation is progressing. Have a look at iomap_begin_write(). Specifically: /* * Now we have a locked folio, before we do anything with it we need to * check that the iomap we have cached is not stale. The inode extent * mapping can change due to concurrent IO in flight (e.g. * IOMAP_UNWRITTEN state can change and memory reclaim could have * reclaimed a previously partially written page at this index after IO * completion before this write reaches this file offset) and hence we * could do the wrong thing here (zero a page range incorrectly or fail * to zero) and corrupt data. */ if (folio_ops && folio_ops->iomap_valid) { bool iomap_valid = folio_ops->iomap_valid(iter->inode, &iter->iomap); if (!iomap_valid) { iter->iomap.flags |= IOMAP_F_STALE; status = 0; goto out_unlock; } } Yup, there's the hook to detect stale cached iomaps. The struct iomap has a iomap->validity_cookie in it, which is an opaque cookie set by the filesytem when it creates the iomap. Here we have locked the folio so guaranteed exclusive access to this file range, and so we pass the iomap with it's cookie back to the filesystem to determine if the iomap is still valid. XFS uses generation numbers in the extent tree to determine if the cached iomap is still valid. ANy change to the extent tree bumps the generation number, and the current generation number is placed in iomap->validity_cookie when the iomap is created. If the generation number on the inode extent tree is different to the number held in the validity_cookie, then the extent tree has changed and the iomap must be considered stale. The iomap iterator then sees IOMAP_F_STALE and generates a new iomap for the remaining range of the write operation. Writeback has the same issue - the iomap_writepage_ctx caches the iomap we obtained for the current writeback, and so if something else changes the extent state while writeback is underway, then that map is stale and needs to be refetched. XFS does this by wrapping the iomap_writepage_ctx with a xfs_writepage_ctx that holds generation numbers so that when writeback calls iomap_writeback_ops->map_blocks(), it can check that the cached iomap is still valid, same as we do in iomap_begin_write(). > Whereas in buffer_head while iterating, we first instantiate/lock the > folio and then call ->map_blocks() to map an extent for the given folio. > > ... So this opens up this window for a race between iomap buffered write > path v/s page mkwrite path for inserting delalloc blocks entries. iomap allows them to to race - the filesystem extent tree needs it's own internal locking to serialise lookups and modifications of the extent tree, whilst the data modifications and page cache state changes are serialised by the folio lock. That's why iomap_begin_write() checks that the iomap is still valid only after it has a locked folio it is ready to write data into. Remeber that delalloc extents need to be inserted into the filesystem internal tree when ->iomap_begin() creates them. Hence anything that races to write over that same range range will only create the delalloc extent once - the second operation will simply find the existing delalloc extent the first operation created... > > the race between buffered write path and fallocate path > > was discovered while I was analyzing the code, so I'm not sure if it could > > be caught by xfstests now, at least I haven't noticed this problem so far. > > > > Did you mean the race between page fault path and fallocate path here? > Because buffered write path and fallocate path should not have any race > since both takes the inode_lock. I guess you meant page fault path and > fallocate path for which you wrote this patch too :) > > I am surprised, why we cannot see the this race between page mkwrite and > fallocate in fstests for inserting da entries to extent status cache. Finding workloads that hit these sorts of races reliably is -real hard-. Read the commit message in commit d7b64041164c ("iomap: write iomap validity checks"), especially this link: https://lore.kernel.org/linux-xfs/20220817093627.GZ3600936@xxxxxxxxxxxxxxxxxxx/ And this comment I made in a followup email: " [....] and it points out that every filesystem using iomap for multi-page extent maps will need to implement iomap invalidation detection in some way." > Because the race you identified looks like a legitimate race and is > mostly happening since ext4_da_map_blocks() was not doing the right > thing. > ... looking at the src/holetest, it doesn't really excercise this path. > So maybe we can writing such fstest to trigger this race. We have a regression test that exercises folio_ops->iomap_valid() functionality: xfs/559. It uses the XFS error injection infrastructure to add a strategic delay which we placed in xfs_iomap_valid() so that we can hold an iomap cached for an arbitrary period of time to allow writeback and page cache reclaim to do their stuff to cause the extent map held by the write to become stale. It also uses ftrace to capture the tracepoint that tells us that the invalid iomap state was seen and IOMAP_F_STALE behaviour triggered. This could be turned into a generic test, but there's a lot of missing infrastructure bits to do it.... Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx