Re: [PATCH 0/3] btrfs: make subpage reader/writer counter to be sector aware

[Boris Burkov <boris@bur.io>]

On Thu, Feb 15, 2024 at 07:39:15AM +1030, Qu Wenruo wrote:
> 
> 
> 在 2024/2/15 04:51, Boris Burkov 写道:
> > On Wed, Feb 14, 2024 at 02:34:33PM +1030, Qu Wenruo wrote:
> > > This can be fetched from github, and the branch would be utilized for
> > > all newer subpage delalloc update to support full sector sized
> > > compression and zoned:
> > > https://github.com/adam900710/linux/tree/subpage_delalloc
> > > 
> > > Currently we just trace subpage reader/writer counter using an atomic.
> > > 
> > > It's fine for the current subpage usage, but for the future, we want to
> > > be aware of which subpage sector is locked inside a page, for proper
> > > compression (we only support full page compression for now) and zoned support.
> > 
> > The logic of the patches seems good and self-consistent to me, I don't
> > see any issues.
> > 
> > However, I think it would be helpful to at least see the client code to
> > motivate the bitmap a bit more for the ignorant :)
> 
> Sure, if needed I can include them into the incoming subpage delalloc
> patchset.
> 
> > 
> > Also, from a semi-cursory inspection, it looks like this relies on
> > extent locking to ensure that multiple threads don't collide on the
> > subpage bitmap, is that correct?
> 
> The current plan is to make find_lock_delalloc_range() to always lock
> all the ranges inside the page, at least beyond the end of the page.
> 
> The main work flow would look like this:
> 
> find_lock_delalloc_range()
> {
> 	int cur = page_offset(page);
> 
> 	/*
> 	 * Subpage, already locked, just grab the next locked range
> 	 * using the locked bitmap.
> 	 */
> 	if (btrfs_is_subpage() && write_count > 0)
> 		return grab_the_next_locked_range();
> 
> 	while (cur < page_end(page)) {
> 		/*
> 		 * The old find and lock code, including
> 		 * the extent locking
> 		 */
> 		cur = locked_range_end();
> 	}
> 	*start = the_first_locked_range_start;
> 	*end = the_first_locked_range_end;
> }
> 
> So for non-subpage cases, it's the same.
> For subpage cases, the page would be kept locked until all its subpage
> sectors are written.
> (But would need extra cleanup if we hit some error during subpage sector
> write)
> 
> And the above workflow is still being coded, not yet tested to see if
> there is anything fundamentally wrong, thus it may change.
> 
> > You should check with Josef that his
> > plans with getting rid of the extent locking don't clash with this.
> 
> It would still conflict, but the extent locking part would be the same
> as usual, so I believe the conflict can be easily resolved.
> 
> And I'm pretty happy to help solving the conflicts if needed.

By conflict, I meant logically/conceptually, not in terms of git merge
conflicts.

Right now, AFAICT, your code relies on the fact that the extent is
locked to ensure that two reads don't trip on the bitmap. If Josef takes
that out, does the assumption still hold? Page lock gets taken after
modifying the bitmap, right?

Sorry if I am misunderstanding you.

> 
> Thanks,
> Qu
> 
> > 
> > Thanks,
> > Boris
> > 
> > > 
> > > So here we introduce a new bitmap, called locked bitmap, to trace which
> > > sector is locked for read/write.
> > > 
> > > And since reader/writer are both exclusive (to each other and to the same
> > > type of lock), we can safely use the same bitmap for both reader and
> > > writer.
> > > 
> > > In theory we can use the bitmap (the weight of the locked bitmap) to
> > > indicate how many bytes are under reader/write lock, but it's not
> > > possible yet:
> > > 
> > > - No weight support for bitmap range
> > >    The bitmap API only provides bitmap_weight(), which always starts at
> > >    bit 0.
> > > 
> > > - Need to distinguish read/write lock
> > > 
> > > Thus we still keep the reader/writer atomic counter.
> > > 
> > > Qu Wenruo (3):
> > >    btrfs: unexport btrfs_subpage_start_writer() and
> > >      btrfs_subpage_end_and_test_writer()
> > >    btrfs: subpage: make reader lock to utilize bitmap
> > >    btrfs: subpage: make writer lock to utilize bitmap
> > > 
> > >   fs/btrfs/subpage.c | 70 ++++++++++++++++++++++++++++++++++++----------
> > >   fs/btrfs/subpage.h | 16 +++++++----
> > >   2 files changed, 66 insertions(+), 20 deletions(-)
> > > 
> > > --
> > > 2.43.1
> > > 
> >