| Message ID | cover.1707883446.git.wqu@suse.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | btrfs: make subpage reader/writer counter to be sector aware | expand |
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 :) 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? You should check with Josef that his plans with getting rid of the extent locking don't clash with this. 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 >
在 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. 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 >> >
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 > > > > >
在 2024/2/15 07:55, Boris Burkov 写道: > 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. Read and write are ensured to not happen on the same page by page lock/writeback flags. If Josef takes extent locking away, it's still fine. > If Josef takes > that out, does the assumption still hold? Page lock gets taken after > modifying the bitmap, right? The page is already locked before taking the bitmap. The page is locked in extent_write_cache_pages(), while we take that locked page and update the bitmap in find_lock_delalloc_range(). extent_write_cache_pages() |- folio_trylock() << Page locked |- __extent_writepages() |- writepage_delalloc() |- find_lock_delalloc_range() << Update lock bitmap The whole change is to address the page unlock part, so that the whole page can only be unlocked by the last delalloc range of the page. Thanks, Qu > > 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 >>>> >>>
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. 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(-)