Why does mpage_writepage() not handle locked buffers?

Why does mpage_writepage() not handle locked buffers?

[Matthew Wilcox]

I tried this:

-static int blkdev_writepage(struct page *page, struct writeback_control *wbc)
+static int blkdev_writepages(struct address_space *mapping,
+               struct writeback_control *wbc)
 {
-       return block_write_full_page(page, blkdev_get_block, wbc);
+       return mpage_writepages(mapping, wbc, blkdev_get_block);
 }

and I hit the BUG_ON(buffer_locked(bh)); in __mpage_writepage() which
has been there since you added it in 2002.

block_write_full_page() handles this fine, so why isn't this

	if (buffer_locked(bh))
		goto confused;

Is the thought that we hold the folio locked, therefore no buffers
should be locked at this time?  I don't know the rules.

In case it's relevant, I hit this while running xfstests generic/013
with ext4 and the stack backtrace is:

__mpage_writepage+0x18d/0x7c0
 write_cache_pages+0x17e/0x480
 mpage_writepages+0x44/0x80
 blkdev_writepages+0x10/0x20
 do_writepages+0xa9/0x150
 filemap_fdatawrite+0x70/0x80
 sync_bdevs+0x151/0x160
 ksys_sync+0x55/0x80

Re: Why does mpage_writepage() not handle locked buffers?

[Christoph Hellwig]

On Fri, Dec 15, 2023 at 05:43:53AM +0000, Matthew Wilcox wrote:
> block_write_full_page() handles this fine, so why isn't this
> 
> 	if (buffer_locked(bh))
> 		goto confused;
> 
> Is the thought that we hold the folio locked, therefore no buffers
> should be locked at this time?  I don't know the rules.

That would be my guess.  For writeback on the fs mapping no one
else should ever have locked the buffer.  For the bdev mapping
buffer locking isn't completely controlled by the bdevfs, but also
by any mounted fs using the buffer cache.  So from my POV your
above change should be ok, but it'll need a big fat comment so
that the next person seeing it in 20 years isn't as confused as
you are now :)

Re: Why does mpage_writepage() not handle locked buffers?

[Matthew Wilcox]

On Fri, Dec 15, 2023 at 02:49:55AM -0800, Christoph Hellwig wrote:
> On Fri, Dec 15, 2023 at 05:43:53AM +0000, Matthew Wilcox wrote:
> > block_write_full_page() handles this fine, so why isn't this
> > 
> > 	if (buffer_locked(bh))
> > 		goto confused;
> > 
> > Is the thought that we hold the folio locked, therefore no buffers
> > should be locked at this time?  I don't know the rules.
> 
> That would be my guess.  For writeback on the fs mapping no one
> else should ever have locked the buffer.  For the bdev mapping
> buffer locking isn't completely controlled by the bdevfs, but also
> by any mounted fs using the buffer cache.  So from my POV your
> above change should be ok, but it'll need a big fat comment so
> that the next person seeing it in 20 years isn't as confused as
> you are now :)

Thanks!  Now I'm thinking that it's not OK to call mpage_writepages().
__block_write_full_folio() takes the buffer lock and holds it across the
I/O (it seems that we never split the buffer lock into buffer writeback
like we did the page lock?)  So filesystems may have an expectation that
locking the buffer synchronises with writeback.  Perhaps it's safest to
just call block_write_full_page() in a loop for blockdev?

I wrote up this before realising that I'd misread the code; adding it
here for posterity.

For filesystems, we know that the filesystem will not lock the buffer
since the writeback path holds the folio locked at this point.  For block
devices, the filesystem mounted on it may lock the buffer without locking
the folio first (observed with both ext2 and ext4).  If we find the
buffer locked, just fall back to block_write_full_page() which handles
potentially locked buffers correctly.  It's fine if we win the race and
the filesystem locks the buffer after this point; block_write_full_page()
doesn't hold the buffer locked across the I/O, so the filesystem has no
expectations that locking the buffer will wait for data writeback.