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.
IOWs, getting rid of the state lock out of the uptodate range
changes does not actually get rid of it from the buffered IO patch.
we still have to take it to update the dirty range, and so there's
an obvious way to optimise the state lock usage without changing any
of the bitmap access serialisation behaviour. i.e. We combine the
uptodate and dirty range updates in __iomap_write_end() into a
single lock context such as:
iomap_set_range_dirty_uptodate()
{
struct iomap_folio_state *ifs = folio->private;
struct inode *inode:
unsigned int blks_per_folio;
unsigned int first_blk;
unsigned int last_blk;
unsigned int nr_blks;
unsigned long flags;
if (!ifs)
return;
inode = folio->mapping->host;
blks_per_folio = i_blocks_per_folio(inode, folio);
first_blk = (off >> inode->i_blkbits);
last_blk = (off + len - 1) >> inode->i_blkbits;
nr_blks = last_blk - first_blk + 1;
spin_lock_irqsave(&ifs->state_lock, flags);
bitmap_set(ifs->state, first_blk, nr_blks);
bitmap_set(ifs->state, first_blk + blks_per_folio, nr_blks);
spin_unlock_irqrestore(&ifs->state_lock, flags);
}
This means we calculate the bitmap offsets only once, we take the
state lock only once, and we don't do anything if there is no
sub-folio state.
If we then fix the __iomap_write_begin() code as Willy pointed out
to elide the erroneous uptodate range update, then we end up only
taking the state lock once per buffered write instead of 3 times per
write.
This patch only reduces it to twice per buffered write, so doing the
above should provide even better performance without needing to
change the underlying serialisation mechanism at all.
-Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
