The patchset can be fetched from github:
https://github.com/adam900710/linux/tree/compression
The branch is based on the previously submitted subpage enablement
patchset.
The target merge window is v5.16 or v5.17.
=== What's working ===
Delalloc range which is fully page aligned can be compressed with
64K page size and 4K sector size (AKA, subpage).
With current patchset, it can pass most "compress" test group, except
btrfs/106, whose golden output is bound to 4K page size, thus test case
needs to be updated.
And as a basic requirement, 4K page size systems still pass the regular
fstests runs.
=== What's not working ===
Delalloc range not fully page aligned will not go through compression.
That's to say, the following inode will go through different write path:
0 32K 64K 96K 128K
|///////////////| |///////|
| \- Will not be compressed
|
\- Will be compressed
This will reduce the chance of compression obviously.
But all involved patches will be the basis for later sector perfect
compression support.
The limitation is mostly introduced by two factors:
- How we handle the locked page of a async cow delalloc range
Currently we unlock the first page unconditionally.
Even with the patchset, we still follows the behavior.
This means we can't have two async cow range shares the same
page.
This can be enhanced to use subpage::writers, but the next
problem will prevent us doing so.
- No way to ensure an async cow range not to unlock the page while
we still have delalloc range in the page
This is caused by how we run delalloc range in a page.
For regular sectorsize, it's not a problem as we have at most one
sector for a page.
But for subpage case, we can have multiple sectors in one page.
If we submit an async cow, it may try to unlock the page while
we are still running the next delalloc range of the page.
The correct way here is to find and lock all delalloc range inside a
page, update the subpage::writers properly, then run each delalloc
range, so that the page won't be unlocked half way.
=== Patch structure ===
Patch 01~04: Small and safe cleanups
Patch 05: Make compressed readahead to be subpage compatble
Patch 06~14: Optimize compressed read/write path to determine stripe
boundary in a per-bio base
Patch 15~16: Extra code refactor/cleanup for compressed path
Patch 17~25: Make compressed write path to be subpage compatible
Patch 26: Enable limited subpage compressed write support
Patch 01~16 may be a good candidate for early merge, as real heavy
lifting part starts at patch 17.
While patch 01~04 are really small and safe cleanups, which can be
merged even earlier than subpage enablement patchset.
While the patches 06~14 is quite some refactor, it may be needed for the
read-only support for compression.
As the read-time bio split is also a critical part for read-only
compressed data support.
I don't have any good idea to push those read path fixes to v5.15
branches for now.
Maybe I need to craft a hot-fix for read-write support.
Changelog:
v2:
- Rebased to latest misc-next
Conflicts are caused by compact subpage bitmaps and refactored bool
parameters for @uptodate.
All tested on aarch64 machines.
v3:
- Fixed a bug in copy_compressed_data_to_page()
When the compressed data and its headers can fill the last page,
we will call memset() with @dest = NULL while @size = 0.
This can causee NULL pointer dereference on some systems.
The latest fix will remove the "cur_page = NULL" assignment to ensure
@cur_page is always pointing to a valid page, and skip the page tail
padding if we filled the last page.
Qu Wenruo (26):
btrfs: remove unused parameter @nr_pages in add_ra_bio_pages()
btrfs: remove unnecessary parameter @delalloc_start for
writepage_delalloc()
btrfs: use async_chunk::async_cow to replace the confusing pending
pointer
btrfs: don't pass compressed pages to
btrfs_writepage_endio_finish_ordered()
btrfs: make add_ra_bio_pages() to be subpage compatible
btrfs: introduce compressed_bio::pending_sectors to trace compressed
bio more elegantly
btrfs: add subpage checked bitmap to make PageChecked flag to be
subpage compatible
btrfs: handle errors properly inside btrfs_submit_compressed_read()
btrfs: handle errors properly inside btrfs_submit_compressed_write()
btrfs: introduce submit_compressed_bio() for compression
btrfs: introduce alloc_compressed_bio() for compression
btrfs: make btrfs_submit_compressed_read() to determine stripe
boundary at bio allocation time
btrfs: make btrfs_submit_compressed_write() to determine stripe
boundary at bio allocation time
btrfs: remove unused function btrfs_bio_fits_in_stripe()
btrfs: refactor submit_compressed_extents()
btrfs: cleanup for extent_write_locked_range()
btrfs: make compress_file_range() to be subpage compatible
btrfs: make btrfs_submit_compressed_write() to be subpage compatible
btrfs: make end_compressed_bio_writeback() to be subpage compatble
btrfs: make extent_write_locked_range() to be subpage compatible
btrfs: extract uncompressed async extent submission code into a new
helper
btrfs: rework lzo_compress_pages() to make it subpage compatible
btrfs: teach __extent_writepage() to handle locked page differently
btrfs: allow page to be unlocked by btrfs_page_end_writer_lock() even
if it's locked by plain page_lock()
btrfs: don't run delalloc range which is beyond the locked_page to
prevent deadlock for subpage compression
btrfs: only allow subpage compression if the range is fully page
aligned
fs/btrfs/compression.c | 681 ++++++++++++++++++-------------
fs/btrfs/compression.h | 4 +-
fs/btrfs/ctree.h | 2 -
fs/btrfs/extent_io.c | 123 ++++--
fs/btrfs/extent_io.h | 3 +-
fs/btrfs/file.c | 20 +-
fs/btrfs/free-space-cache.c | 6 +-
fs/btrfs/inode.c | 456 +++++++++++----------
fs/btrfs/lzo.c | 270 ++++++------
fs/btrfs/reflink.c | 2 +-
fs/btrfs/subpage.c | 102 ++++-
fs/btrfs/subpage.h | 4 +
fs/btrfs/tests/extent-io-tests.c | 12 +-
13 files changed, 1000 insertions(+), 685 deletions(-)
--
2.33.0
Variable @nr_pages only get increased but never used.
Remove it.
Signed-off-by: Qu Wenruo <redacted>
---
fs/btrfs/compression.c | 2 --
1 file changed, 2 deletions(-)
In function __extent_writepage() we always pass page start to
@delalloc_start for writepage_delalloc().
Thus we don't really need @delalloc_start parameter as we can extract it
from @page.
So this patch will remove @delalloc_start parameter and make
__extent_writepage() to declare @page_start and @page_end as const.
Signed-off-by: Qu Wenruo <redacted>
---
fs/btrfs/extent_io.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
For structure async_chunk, we use a very strange member layout to grab
structure async_cow who owns this async_chunk.
At initialization, it goes like this:
async_chunk[i].pending = &ctx->num_chunks;
Then at async_cow_free() we do a super weird freeing:
/*
* Since the pointer to 'pending' is at the beginning of the array of
* async_chunk's, freeing it ensures the whole array has been freed.
*/
if (atomic_dec_and_test(async_chunk->pending))
kvfree(async_chunk->pending);
This is absolutely an abuse of kvfree().
Replace async_chunk::pending with async_chunk::async_cow, so that we can
grab the async_cow structure directly, without this strange dancing.
And with this change, there is no requirement for any specific member
location.
Signed-off-by: Qu Wenruo <redacted>
---
fs/btrfs/inode.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
@@ -455,11 +455,10 @@ struct async_chunk {structlist_headextents;structcgroup_subsys_state*blkcg_css;structbtrfs_workwork;-atomic_t*pending;+structasync_cow*async_cow;};structasync_cow{-/* Number of chunks in flight; must be first in the structure */atomic_tnum_chunks;structasync_chunkchunks[];};
Since async_extent holds the compressed page, it would trigger the new
ASSERT() in btrfs_mark_ordered_io_finished() which checks the range is
inside the page.
Now btrfs_writepage_endio_finish_ordered() can accept @page == NULL,
just pass NULL to btrfs_writepage_endio_finish_ordered().
Signed-off-by: Qu Wenruo <redacted>
---
fs/btrfs/inode.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
[BUG]
If we remove the subpage limitation in add_ra_bio_pages(), then read a
compressed extent which has part of its range in next page, like the
following inode layout:
0 32K 64K 96K 128K
|<--------------|-------------->|
Btrfs will trigger ASSERT() in endio function:
assertion failed: atomic_read(&subpage->readers) >= nbits
------------[ cut here ]------------
kernel BUG at fs/btrfs/ctree.h:3431!
Internal error: Oops - BUG: 0 [#1] SMP
Workqueue: btrfs-endio btrfs_work_helper [btrfs]
Call trace:
assertfail.constprop.0+0x28/0x2c [btrfs]
btrfs_subpage_end_reader+0x148/0x14c [btrfs]
end_page_read+0x8c/0x100 [btrfs]
end_bio_extent_readpage+0x320/0x6b0 [btrfs]
bio_endio+0x15c/0x1dc
end_workqueue_fn+0x44/0x64 [btrfs]
btrfs_work_helper+0x74/0x250 [btrfs]
process_one_work+0x1d4/0x47c
worker_thread+0x180/0x400
kthread+0x11c/0x120
ret_from_fork+0x10/0x30
---[ end trace c8b7b552d3bb408c ]---
[CAUSE]
When we read the page range [0, 64K), we find it's a compressed extent,
and we will try to add extra pages in add_ra_bio_pages() to avoid
reading the same compressed extent.
But when we add such page into the read bio, it doesn't follow the
behavior of btrfs_do_readpage() to properly set subpage::readers.
This means, for page [64K, 128K), its subpage::readers is still 0.
And when endio is executed on both pages, since page [64K, 128K) has 0
subpage::readers, it triggers above ASSERT()
[FIX]
Function add_ra_bio_pages() is far from subpage compatible, it always
assume PAGE_SIZE == sectorsize, thus when it skip to next range it
always just skip PAGE_SIZE.
Make it subpage compatible by:
- Skip to next page properly when needed
If we find there is already a page cache, we need to skip to next
page.
For that case, we shouldn't just skip PAGE_SIZE bytes, but use
@pg_index to calculate the next bytenr and continue.
- Only add the page range covered by current extent map
We need to calculate which range is covered by current extent map and
only add that part into the read bio.
- Update subpage::readers before submitting the bio
- Use proper cursor other than confusing @last_offset
- Calculate the missed threshold based on sector size
It's no longer using missed pages, as for 64K page size, we have at
most 3 pages to skip. (If aligned only 2 pages)
- Add ASSERT() to make sure our bytenr is always aligned
- Add comment for the function
Add an especial note for subpage case, as the function won't really
work well for subpage cases.
Signed-off-by: Qu Wenruo <redacted>
---
fs/btrfs/compression.c | 89 +++++++++++++++++++++++++++---------------
fs/btrfs/extent_io.c | 1 +
2 files changed, 59 insertions(+), 31 deletions(-)
@@ -541,13 +541,24 @@ static u64 bio_end_offset(struct bio *bio)returnpage_offset(last->bv_page)+last->bv_len+last->bv_offset;}+/*+*Addextrapagesinthesamecompressedfileextentsothatwedon't+*needtore-readthesameextentagainandagain.+*+*NOTE:thiswon'tworkwellforsubpage,asforsubpageread,welockthe+*fullpagethensubmitbioforeachcompressed/regularextents.+*+*Thismeans,ifwehaveseveralsectorsinthesamepagepointstothesame+*on-diskcompresseddata,wewillre-readthesameextentmanytimesand+*thisfunctioncanonlyhelpforthenextpage.+*/staticnoinlineintadd_ra_bio_pages(structinode*inode,u64compressed_end,structcompressed_bio*cb){+structbtrfs_fs_info*fs_info=btrfs_sb(inode->i_sb);unsignedlongend_index;-unsignedlongpg_index;-u64last_offset;+u64cur=bio_end_offset(cb->orig_bio);u64isize=i_size_read(inode);intret;structpage*page;
@@ -555,10 +566,8 @@ static noinline int add_ra_bio_pages(struct inode *inode,structaddress_space*mapping=inode->i_mapping;structextent_map_tree*em_tree;structextent_io_tree*tree;-u64end;-intmisses=0;+intsectors_missed=0;-last_offset=bio_end_offset(cb->orig_bio);em_tree=&BTRFS_I(inode)->extent_tree;tree=&BTRFS_I(inode)->io_tree;
@@ -577,18 +586,29 @@ static noinline int add_ra_bio_pages(struct inode *inode,end_index=(i_size_read(inode)-1)>>PAGE_SHIFT;-while(last_offset<compressed_end){-pg_index=last_offset>>PAGE_SHIFT;+while(cur<compressed_end){+u64page_end;+u64pg_index=cur>>PAGE_SHIFT;+u32add_size;if(pg_index>end_index)break;page=xa_load(&mapping->i_pages,pg_index);if(page&&!xa_is_value(page)){-misses++;-if(misses>4)+sectors_missed+=(PAGE_SIZE-offset_in_page(cur))>>+fs_info->sectorsize_bits;++/* Beyond threshold, no need to continue */+if(sectors_missed>4)break;-gotonext;++/*+*Jumptonextpagestartaswealreadyhavepagefor+*currentoffset.+*/+cur=(pg_index<<PAGE_SHIFT)+PAGE_SIZE;+continue;}page=__page_cache_alloc(mapping_gfp_constraint(mapping,
@@ -598,14 +618,11 @@ static noinline int add_ra_bio_pages(struct inode *inode,if(add_to_page_cache_lru(page,mapping,pg_index,GFP_NOFS)){put_page(page);-gotonext;+/* There is already a page, skip to page end */+cur=(pg_index<<PAGE_SHIFT)+PAGE_SIZE;+continue;}-/*-*atthispoint,wehavealockedpageinthepagecache-*forthesebytesinthefile.But,wehavetomake-*suretheymaptothiscompressedextentondisk.-*/ret=set_page_extent_mapped(page);if(ret<0){unlock_page(page);
@@ -613,18 +630,22 @@ static noinline int add_ra_bio_pages(struct inode *inode,break;}-end=last_offset+PAGE_SIZE-1;-lock_extent(tree,last_offset,end);+page_end=(pg_index<<PAGE_SHIFT)+PAGE_SIZE-1;+lock_extent(tree,cur,page_end);read_lock(&em_tree->lock);-em=lookup_extent_mapping(em_tree,last_offset,-PAGE_SIZE);+em=lookup_extent_mapping(em_tree,cur,page_end+1-cur);read_unlock(&em_tree->lock);-if(!em||last_offset<em->start||-(last_offset+PAGE_SIZE>extent_map_end(em))||+/*+*Atthispoint,wehavealockedpageinthepagecache+*forthesebytesinthefile.But,wehavetomake+*suretheymaptothiscompressedextentondisk.+*/+if(!em||cur<em->start||+(cur+fs_info->sectorsize>extent_map_end(em))||(em->block_start>>9)!=cb->orig_bio->bi_iter.bi_sector){free_extent_map(em);-unlock_extent(tree,last_offset,end);+unlock_extent(tree,cur,page_end);unlock_page(page);put_page(page);break;
@@ -642,19 +663,25 @@ static noinline int add_ra_bio_pages(struct inode *inode,}}+add_size=min(em->start+em->len,page_end+1)-cur;ret=bio_add_page(cb->orig_bio,page,-PAGE_SIZE,0);--if(ret==PAGE_SIZE){-put_page(page);-}else{-unlock_extent(tree,last_offset,end);+add_size,offset_in_page(cur));+if(ret!=add_size){+unlock_extent(tree,cur,page_end);unlock_page(page);put_page(page);break;}-next:-last_offset+=PAGE_SIZE;+/*+*Ifit'ssubpage,wealsoneedtoincreaseits+*subpage::readersnumbre,asatendiowewill+*decreasesubpage::readersandtounlockthepage.+*/+if(fs_info->sectorsize<PAGE_SIZE)+btrfs_subpage_start_reader(fs_info,page,+cur,add_size);+put_page(page);+cur+=add_size;}return0;}
CC [M] fs/btrfs/tests/extent-map-tests.o
fs/btrfs/compression.c: In function ‘add_ra_bio_pages’:
fs/btrfs/compression.c:680:25: error: implicit declaration of function ‘btrfs_subpage_start_reader’ [-Werror=implicit-function-declaration]
680 | btrfs_subpage_start_reader(fs_info, page, cur, add_size);
|
Fails when compiled with that patch on top.
Missing #include "subpage.h"
CC [M] fs/btrfs/tests/extent-map-tests.o
fs/btrfs/compression.c: In function ‘add_ra_bio_pages’:
fs/btrfs/compression.c:680:25: error: implicit declaration of function ‘btrfs_subpage_start_reader’ [-Werror=implicit-function-declaration]
680 | btrfs_subpage_start_reader(fs_info, page, cur, add_size);
|
Fails when compiled with that patch on top.
Missing #include "subpage.h"
Oh, I added that in later patches, without doing a per-patch compiling
tests...
Sorry for that.
Thanks,
Qu
For btrfs_submit_compressed_read() and btrfs_submit_compressed_write(),
we have a pretty weird dance around compressed_bio::pending_bios:
btrfs_submit_compressed_read/write()
{
cb = kmalloc()
refcount_set(&cb->pending_bios, 0);
bio = btrfs_alloc_bio();
/* NOTE here, we haven't yet submitted any bio */
refcount_set(&cb->pending_bios, 1);
for (pg_index = 0; pg_index < cb->nr_pages; pg_index++) {
if (submit) {
/* Here we submit bio, but we always have one
* extra pending_bios */
refcount_inc(&cb->pending_bios);
ret = btrfs_map_bio();
}
}
/* Submit the last bio */
ret = btrfs_map_bio();
}
There are two reasons why we do this:
- compressed_bio::pending_bios is a refcount
Thus if it's reduced to 0, it can not be increased again.
- To ensure the compressed_bio is not freed by some submitted bios
If the submitted bio is finished before the next bio submitted,
we can free the compressed_bio completely.
But the above code is sometimes confusing, and we can do it better by
just introduce a new member, compressed_bio::pending_sectors.
Now we use compressed_bio::pending_sectors to indicate whether we have any
pending sectors under IO or not yet submitted.
If pending_sectors == 0, we're definitely the last bio of compressed_bio,
and is OK to release the compressed bio.
Now the workflow looks like this:
btrfs_submit_compressed_read/write()
{
cb = kmalloc()
atomic_set(&cb->pending_bios, 0);
refcount_set(&cb->pending_sectors,
compressed_len >> sectorsize_bits);
bio = btrfs_alloc_bio();
for (pg_index = 0; pg_index < cb->nr_pages; pg_index++) {
if (submit) {
refcount_inc(&cb->pending_bios);
ret = btrfs_map_bio();
}
}
/* Submit the last bio */
refcount_inc(&cb->pending_bios);
ret = btrfs_map_bio();
}
For now we still need pending_bios for later error handling, but will
remove pending_bios eventually after properly handling the errors.
Signed-off-by: Qu Wenruo <redacted>
---
fs/btrfs/compression.c | 78 ++++++++++++++++++++++++------------------
fs/btrfs/compression.h | 5 ++-
2 files changed, 49 insertions(+), 34 deletions(-)
@@ -192,6 +192,39 @@ static int check_compressed_csum(struct btrfs_inode *inode, struct bio *bio,return0;}+/*+*Reducebioandioaccountingforacompressed_biowithitscorespondingbio.+*+*Returntrueifthereisnopendingbionorio.+*Returnfalseotherwise.+*/+staticbooldec_and_test_compressed_bio(structcompressed_bio*cb,+structbio*bio)+{+structbtrfs_fs_info*fs_info=btrfs_sb(cb->inode->i_sb);+unsignedintbi_size=0;+boollast_io=false;+structbio_vec*bvec;+structbvec_iter_alliter_all;++/*+*Atendiotime,bi_iter.bi_sizedoesn'trepresenttherealbiosize.+*Thusherewehavetoiteratethroughallsegmentstograbcorrect+*biosize.+*/+bio_for_each_segment_all(bvec,bio,iter_all)+bi_size+=bvec->bv_len;++if(bio->bi_status)+cb->errors=1;++ASSERT(bi_size&&bi_size<=cb->compressed_len);+last_io=refcount_sub_and_test(bi_size>>fs_info->sectorsize_bits,+&cb->pending_sectors);+atomic_dec(&cb->pending_bios);+returnlast_io;+}+/* when we finish reading compressed pages from the disk, we*decompressthemandthenrunthebioend_ioroutinesonthe*decompressedpages(intheinodeaddressspace).
@@ -211,13 +244,7 @@ static void end_compressed_bio_read(struct bio *bio)unsignedintmirror=btrfs_bio(bio)->mirror_num;intret=0;-if(bio->bi_status)-cb->errors=1;--/* if there are more bios still pending for this compressed-*extent,justexit-*/-if(!refcount_dec_and_test(&cb->pending_bios))+if(!dec_and_test_compressed_bio(cb,bio))gotoout;/*
@@ -335,13 +362,7 @@ static void end_compressed_bio_write(struct bio *bio)structpage*page;unsignedintindex;-if(bio->bi_status)-cb->errors=1;--/* if there are more bios still pending for this compressed-*extent,justexit-*/-if(!refcount_dec_and_test(&cb->pending_bios))+if(!dec_and_test_compressed_bio(cb,bio))gotoout;/* ok, we're the last bio for this extent, step one is to
@@ -29,7 +29,10 @@ struct btrfs_inode;structcompressed_bio{/* number of bios pending for this compressed extent */-refcount_tpending_bios;+atomic_tpending_bios;++/* Number of sectors with unfinished IO (unsubmitted or unfinished) */+refcount_tpending_sectors;/* Number of compressed pages in the array */unsignedintnr_pages;
There are quite some BUG_ON()s inside btrfs_submit_compressed_read(),
namingly all errors inside the for() loop relies on BUG_ON() to handle
-ENOMEM.
Handle these errors properly by:
- Wait for submitted bios to finish first
Using wake_var_event() APIs to wait without introducing extra memory
overhead inside compressed_bio.
This allows us to wait for any submitted bio to finish, while still
keeps the compressed_bio from being freed.
- Introduce finish_compressed_bio_read() to finish the compressed_bio
- Properly end the bio and finish compressed_bio when error happens
Now in btrfs_submit_compressed_read() even when the bio submission
failed, we can properly handle the error without triggering BUG_ON().
Signed-off-by: Qu Wenruo <redacted>
---
fs/btrfs/compression.c | 133 +++++++++++++++++++++++++----------------
1 file changed, 83 insertions(+), 50 deletions(-)
@@ -223,9 +223,60 @@ static bool dec_and_test_compressed_bio(struct compressed_bio *cb,last_io=refcount_sub_and_test(bi_size>>fs_info->sectorsize_bits,&cb->pending_sectors);atomic_dec(&cb->pending_bios);+/*+*Herewemustwakeupthepossibleerrorhandlerafterallother+*operationson@cbfinished,orwecanracewith+*finish_compressed_bio_*()whichmayfree@cb.+*/+wake_up_var(cb);+returnlast_io;}+staticvoidfinish_compressed_bio_read(structcompressed_bio*cb,+structbio*bio)+{+unsignedintindex;+structpage*page;++/* Release the compressed pages */+for(index=0;index<cb->nr_pages;index++){+page=cb->compressed_pages[index];+page->mapping=NULL;+put_page(page);+}++/* Do io completion on the original bio */+if(cb->errors){+bio_io_error(cb->orig_bio);+}else{+structbio_vec*bvec;+structbvec_iter_alliter_all;++ASSERT(bio);+ASSERT(!bio->bi_status);+/*+*Wehaveverifiedthechecksumalready,setpage+*checkedsotheend_iohandlersknowaboutit+*/+ASSERT(!bio_flagged(bio,BIO_CLONED));+bio_for_each_segment_all(bvec,cb->orig_bio,iter_all){+u64bvec_start=page_offset(bvec->bv_page)++bvec->bv_offset;++btrfs_page_set_checked(btrfs_sb(cb->inode->i_sb),+bvec->bv_page,bvec_start,+bvec->bv_len);+}++bio_endio(cb->orig_bio);+}++/* Finally free the cb struct */+kfree(cb->compressed_pages);+kfree(cb);+}+/* when we finish reading compressed pages from the disk, we*decompressthemandthenrunthebioend_ioroutinesonthe*decompressedpages(intheinodeaddressspace).
@@ -240,8 +291,6 @@ static void end_compressed_bio_read(struct bio *bio){structcompressed_bio*cb=bio->bi_private;structinode*inode;-structpage*page;-unsignedintindex;unsignedintmirror=btrfs_bio(bio)->mirror_num;intret=0;
@@ -276,42 +325,7 @@ static void end_compressed_bio_read(struct bio *bio)csum_failed:if(ret)cb->errors=1;--/* release the compressed pages */-index=0;-for(index=0;index<cb->nr_pages;index++){-page=cb->compressed_pages[index];-page->mapping=NULL;-put_page(page);-}--/* do io completion on the original bio */-if(cb->errors){-bio_io_error(cb->orig_bio);-}else{-structbio_vec*bvec;-structbvec_iter_alliter_all;--/*-*wehaveverifiedthechecksumalready,setpage-*checkedsotheend_iohandlersknowaboutit-*/-ASSERT(!bio_flagged(bio,BIO_CLONED));-bio_for_each_segment_all(bvec,cb->orig_bio,iter_all){-u64bvec_start=page_offset(bvec->bv_page)+-bvec->bv_offset;--btrfs_page_set_checked(btrfs_sb(cb->inode->i_sb),-bvec->bv_page,bvec_start,-bvec->bv_len);-}--bio_endio(cb->orig_bio);-}--/* finally free the cb struct */-kfree(cb->compressed_pages);-kfree(cb);+finish_compressed_bio_read(cb,bio);out:bio_put(bio);}
@@ -890,6 +904,25 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,out:free_extent_map(em);returnret;+last_bio:+comp_bio->bi_status=ret;+/* This is the last bio, endio functions will free @cb */+bio_endio(comp_bio);+returnret;+finish_cb:+if(comp_bio){+comp_bio->bi_status=ret;+bio_endio(comp_bio);+}+wait_var_event(cb,atomic_read(&cb->pending_bios)==0);+/*+*Evenwithpreviousbioended,weshouldstillhaveionotyet+*submitted,thusneedtofinish@cbmanually.+*/+ASSERT(refcount_read(&cb->pending_sectors));+/* Now we are the only one referring @cb, can finish it safely. */+finish_compressed_bio_read(cb,NULL);+returnret;}/*
Although in btrfs we have very limited usage of PageChecked flag, it's
still some page flag not yet subpage compatible.
Fix it by introducing btrfs_subpage::checked_offset to do the convert.
For most call sites, especially for free-space cache, COW fixup and
btrfs_invalidatepage(), they all work in full page mode anyway.
For other call sites, they work as subpage compatible mode.
Some call sites need extra modification:
- btrfs_drop_pages()
Needs extra parameter to get the real range we need to clear checked
flag.
Also since btrfs_drop_pages() will accept pages beyond the dirtied
range, update btrfs_subpage_clamp_range() to handle such case
by setting @len to 0 if the page is beyond target range.
- btrfs_invalidatepage()
We need to call subpage helper before calling __btrfs_releasepage(),
or it will trigger ASSERT() as page->private will be cleared.
- btrfs_verify_data_csum()
In theory we don't need the io_bio->csum check anymore, but it's
won't hurt.
Just change the comment.
Signed-off-by: Qu Wenruo <redacted>
---
fs/btrfs/compression.c | 11 +++++++--
fs/btrfs/file.c | 20 ++++++++++++----
fs/btrfs/free-space-cache.c | 6 ++++-
fs/btrfs/inode.c | 30 +++++++++++------------
fs/btrfs/reflink.c | 2 +-
fs/btrfs/subpage.c | 48 +++++++++++++++++++++++++++++++++++--
fs/btrfs/subpage.h | 2 ++
7 files changed, 92 insertions(+), 27 deletions(-)
@@ -437,9 +437,16 @@ static noinline int btrfs_copy_from_user(loff_t pos, size_t write_bytes,/**unlockspagesafterbtrfs_file_writeisdonewiththem*/-staticvoidbtrfs_drop_pages(structpage**pages,size_tnum_pages)+staticvoidbtrfs_drop_pages(structbtrfs_fs_info*fs_info,+structpage**pages,size_tnum_pages,+u64pos,u64copied){size_ti;+u64block_start=round_down(pos,fs_info->sectorsize);+u64block_len=round_up(pos+copied,fs_info->sectorsize)-+block_start;++ASSERT(block_len<=U32_MAX);for(i=0;i<num_pages;i++){/* page checked is some magic around finding pages that*havebeenmodifiedwithoutgoingthroughbtrfs_set_page_dirty
Just like btrfs_submit_compressed_read(), there are quite some BUG_ON()s
inside btrfs_submit_compressed_write() for the bio submission path.
Fix them using the same method:
- For last bio, just endio the bio
As in that case, one of the endio function of all these submitted bio
will be able to free the compressed_bio
- For half-submitted bio, wait and finish the compressed_bio manually
In this case, as long as all other bio finishes, we're the only one
referring the compressed bio, and can manually finish it.
Signed-off-by: Qu Wenruo <redacted>
---
fs/btrfs/compression.c | 99 +++++++++++++++++++++++++++---------------
1 file changed, 63 insertions(+), 36 deletions(-)
@@ -368,50 +368,56 @@ static noinline void end_compressed_writeback(struct inode *inode,/* the inode may be gone now */}-/*-*dothecleanuponceallthecompressedpageshitthedisk.-*Thiswillclearwritebackonthefilepagesandfreethecompressed-*pages.-*-*Thisalsocallsthewritebackendhooksforthefilepagessothat-*metadataandchecksumscanbeupdatedinthefile.-*/-staticvoidend_compressed_bio_write(structbio*bio)+staticvoidfinish_compressed_bio_write(structcompressed_bio*cb){-structcompressed_bio*cb=bio->bi_private;-structinode*inode;-structpage*page;+structinode*inode=cb->inode;unsignedintindex;-if(!dec_and_test_compressed_bio(cb,bio))-gotoout;--/* ok, we're the last bio for this extent, step one is to-*callbackintotheFSanddoalltheend_iooperations+/*+*Ok,we'rethelastbioforthisextent,steponeisto+*callbackintotheFSanddoalltheend_iooperations.*/-inode=cb->inode;-btrfs_record_physical_zoned(inode,cb->start,bio);btrfs_writepage_endio_finish_ordered(BTRFS_I(inode),NULL,cb->start,cb->start+cb->len-1,!cb->errors);end_compressed_writeback(inode,cb);-/* note, our inode could be gone now */+/* Note, our inode could be gone now *//*-*releasethecompressedpages,thesecamefromalloc_pageand+*Releasethecompressedpages,thesecamefromalloc_pageand*arenotattachedtotheinodeatall*/-index=0;for(index=0;index<cb->nr_pages;index++){-page=cb->compressed_pages[index];+structpage*page=cb->compressed_pages[index];+page->mapping=NULL;put_page(page);}-/* finally free the cb struct */+/* Finally free the cb struct */kfree(cb->compressed_pages);kfree(cb);+}++/*+*dothecleanuponceallthecompressedpageshitthedisk.+*Thiswillclearwritebackonthefilepagesandfreethecompressed+*pages.+*+*Thisalsocallsthewritebackendhooksforthefilepagessothat+*metadataandchecksumscanbeupdatedinthefile.+*/+staticvoidend_compressed_bio_write(structbio*bio)+{+structcompressed_bio*cb=bio->bi_private;++if(!dec_and_test_compressed_bio(cb,bio))+gotoout;++btrfs_record_physical_zoned(cb->inode,cb->start,bio);++finish_compressed_bio_write(cb);out:bio_put(bio);}
@@ -553,23 +559,44 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,atomic_inc(&cb->pending_bios);ret=btrfs_bio_wq_end_io(fs_info,bio,BTRFS_WQ_ENDIO_DATA);-BUG_ON(ret);/* -ENOMEM */+if(ret)+gotolast_bio;if(!skip_sum){ret=btrfs_csum_one_bio(inode,bio,start,1);-BUG_ON(ret);/* -ENOMEM */+if(ret)+gotolast_bio;}ret=btrfs_map_bio(fs_info,bio,0);-if(ret){-bio->bi_status=ret;-bio_endio(bio);-}+if(ret)+gotolast_bio;if(blkcg_css)kthread_associate_blkcg(NULL);return0;+last_bio:+bio->bi_status=ret;+/* One of the bios' endio function will free @cb. */+bio_endio(bio);+returnret;++finish_cb:+if(bio){+bio->bi_status=ret;+bio_endio(bio);+}++wait_var_event(cb,atomic_read(&cb->pending_bios)==0);+/*+*Evenwithpreviousbioended,weshouldstillhaveionotyet+*submitted,thusneedtofinishmanually.+*/+ASSERT(refcount_read(&cb->pending_sectors));+/* Now we are the only one referring @cb, can finish it safely. */+finish_compressed_bio_write(cb);+returnret;}staticu64bio_end_offset(structbio*bio)
The new helper, submit_compressed_bio(), will aggregate the following
work:
- Increase compressed_bio::pending_bios
- Remap the endio function
- Map and submit the bio
Signed-off-by: Qu Wenruo <redacted>
---
fs/btrfs/compression.c | 45 ++++++++++++++++++------------------------
1 file changed, 19 insertions(+), 26 deletions(-)
From: David Sterba <hidden> Date: 2021-10-04 19:42:53
On Mon, Sep 27, 2021 at 03:21:52PM +0800, Qu Wenruo wrote:
quoted hunk
The new helper, submit_compressed_bio(), will aggregate the following
work:
- Increase compressed_bio::pending_bios
- Remap the endio function
- Map and submit the bio
Signed-off-by: Qu Wenruo <redacted>
---
fs/btrfs/compression.c | 45 ++++++++++++++++++------------------------
1 file changed, 19 insertions(+), 26 deletions(-)
Can you please send me an explanation why it's still OK to aggregate the
calls as it changes the order. Originally there's
atomic_inc
btrfs_bio_wq_end_io
btrfs_csum_one_bio
btrfs_map_bio
While in the new code:
btrfs_csum_one_bio
from submit_compressed_bio:
atomic_inc
btrfs_bio_wq_end_io
btrfs_map_bio
So in particular the order of
atomic_inc+btrfs_bio_wq_end_io is in reverse order with
btrfs_csum_one_bio
quoted hunk
@@ -889,7 +887,7 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio, fs_info->sectorsize); sums += fs_info->csum_size * nr_sectors;- ret = btrfs_map_bio(fs_info, comp_bio, mirror_num);+ ret = submit_compressed_bio(fs_info, cb, comp_bio, mirror_num); if (ret) goto finish_cb;
@@ -904,16 +902,11 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio, cur_disk_byte += pg_len; }- atomic_inc(&cb->pending_bios);- ret = btrfs_bio_wq_end_io(fs_info, comp_bio, BTRFS_WQ_ENDIO_DATA);- if (ret)- goto last_bio;- ret = btrfs_lookup_bio_sums(inode, comp_bio, sums); if (ret) goto last_bio;- ret = btrfs_map_bio(fs_info, comp_bio, mirror_num);+ ret = submit_compressed_bio(fs_info, cb, comp_bio, mirror_num);
Same for btrfs_lookup_bio_sums instead of btrfs_csum_one_bio.
On Mon, Sep 27, 2021 at 03:21:52PM +0800, Qu Wenruo wrote:
quoted
The new helper, submit_compressed_bio(), will aggregate the following
work:
- Increase compressed_bio::pending_bios
- Remap the endio function
- Map and submit the bio
Signed-off-by: Qu Wenruo <redacted>
---
fs/btrfs/compression.c | 45 ++++++++++++++++++------------------------
1 file changed, 19 insertions(+), 26 deletions(-)
Can you please send me an explanation why it's still OK to aggregate the
calls as it changes the order. Originally there's
atomic_inc
btrfs_bio_wq_end_io
btrfs_csum_one_bio
btrfs_map_bio
While in the new code:
btrfs_csum_one_bio
from submit_compressed_bio:
atomic_inc
btrfs_bio_wq_end_io
btrfs_map_bio
So in particular the order of
atomic_inc+btrfs_bio_wq_end_io is in reverse order with
btrfs_csum_one_bio
The point is, btrfs_csum_one_bio() does nothing related to bio submission.
It really only fulfill btrfs_bio::csum.
The only important order is atomic_inc() -> btrfs_bio_wq_end_io() ->
btrfs_map_bio().
Thanks,
Qu
quoted
@@ -889,7 +887,7 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio, fs_info->sectorsize); sums += fs_info->csum_size * nr_sectors;- ret = btrfs_map_bio(fs_info, comp_bio, mirror_num);+ ret = submit_compressed_bio(fs_info, cb, comp_bio, mirror_num); if (ret) goto finish_cb;
@@ -904,16 +902,11 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio, cur_disk_byte += pg_len; }- atomic_inc(&cb->pending_bios);- ret = btrfs_bio_wq_end_io(fs_info, comp_bio, BTRFS_WQ_ENDIO_DATA);- if (ret)- goto last_bio;- ret = btrfs_lookup_bio_sums(inode, comp_bio, sums); if (ret) goto last_bio;- ret = btrfs_map_bio(fs_info, comp_bio, mirror_num);+ ret = submit_compressed_bio(fs_info, cb, comp_bio, mirror_num);
Same for btrfs_lookup_bio_sums instead of btrfs_csum_one_bio.
Just aggregate the bio allocation code into one helper, so that we can
replace 4 call sites.
There is one special note for zoned write.
Currently btrfs_submit_compressed_write() will only allocate the first
bio using ZONE_APPEND.
If we have to submit current bio due to stripe boundary, the new bio
allocated will not use ZONE_APPEND.
In theory this should be a bug, but considering zoned mode currently
only support SINGLE profile, which doesn't have any stripe boundary
limit, it should never be a problem.
This function will provide a good entrance for any work which needs to be
done at bio allocation time. Like determining the stripe boundary.
Signed-off-by: Qu Wenruo <redacted>
---
fs/btrfs/compression.c | 90 +++++++++++++++++++++++++++---------------
1 file changed, 58 insertions(+), 32 deletions(-)
@@ -846,11 +867,13 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,/* include any pages we added in add_ra-bio_pages */cb->len=bio->bi_iter.bi_size;-comp_bio=btrfs_bio_alloc(BIO_MAX_VECS);-comp_bio->bi_iter.bi_sector=cur_disk_byte>>SECTOR_SHIFT;-comp_bio->bi_opf=REQ_OP_READ;-comp_bio->bi_private=cb;-comp_bio->bi_end_io=end_compressed_bio_read;+comp_bio=alloc_compressed_bio(cb,cur_disk_byte,REQ_OP_READ,+end_compressed_bio_read);+if(IS_ERR(comp_bio)){+ret=errno_to_blk_status(PTR_ERR(comp_bio));+comp_bio=NULL;+gotofail2;+}for(pg_index=0;pg_index<nr_pages;pg_index++){u32pg_len=PAGE_SIZE;
Currently btrfs_submit_compressed_read() will check
btrfs_bio_fits_in_stripe() each time a new page is going to be added.
Even compressed extent is small, we don't really need to do that for
every page.
This patch will align the behavior to extent_io.c, by determining the
stripe boundary when allocating a bio.
Unlike extent_io.c, in compressed.c we don't need to bother things like
different bio flags, thus no need to re-use bio_ctrl.
Here we just manually introduce new local variable, next_stripe_start,
and teach alloc_compressed_bio() to calculate the stripe boundary.
Then each time we add some page range into the bio, we check if we
reached the boundary.
And if reached, submit it.
Also, since we have @cur_disk_byte to determine whether we're the last
bio, we don't need a explicit last_bio: tag for error handling any more.
And we can use @cur_disk_byte to track which range has been added to
bio, we can also use @cur_disk_byte to calculate the wait condition, no
need for @pending_bios.
Signed-off-by: Qu Wenruo <redacted>
---
fs/btrfs/compression.c | 164 +++++++++++++++++++++++------------------
1 file changed, 93 insertions(+), 71 deletions(-)
@@ -867,39 +884,62 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,/* include any pages we added in add_ra-bio_pages */cb->len=bio->bi_iter.bi_size;-comp_bio=alloc_compressed_bio(cb,cur_disk_byte,REQ_OP_READ,-end_compressed_bio_read);-if(IS_ERR(comp_bio)){-ret=errno_to_blk_status(PTR_ERR(comp_bio));-comp_bio=NULL;-gotofail2;-}--for(pg_index=0;pg_index<nr_pages;pg_index++){-u32pg_len=PAGE_SIZE;-intsubmit=0;+while(cur_disk_byte<disk_bytenr+compressed_len){+u64offset=cur_disk_byte-disk_bytenr;+unsignedintindex=offset>>PAGE_SHIFT;+unsignedintreal_size;+unsignedintadded;+structpage*page=cb->compressed_pages[index];+boolsubmit=false;+/* Allocate new bio if submitted or not yet allocated */+if(!comp_bio){+comp_bio=alloc_compressed_bio(cb,cur_disk_byte,+REQ_OP_READ,end_compressed_bio_read,+&next_stripe_start);+if(IS_ERR(comp_bio)){+ret=errno_to_blk_status(PTR_ERR(comp_bio));+comp_bio=NULL;+gotofinish_cb;+}+}/*-*Tohandlesubpagecase,weneedtomakesurethebioonly-*coverstherangeweneed.-*-*Ifwe'reatthelastpage,truncatethelengthtoonlycover-*theremainingpart.+*Weshouldneverreachnext_stripe_startstartaswewill+*submitcomp_biowhenreachtheboundaryimmediately.+*/+ASSERT(cur_disk_byte!=next_stripe_start);+/*+*Wehavevariouslimitontherealreadsize:+*-stripeboundary+*-pageboundary+*-compressedlengthboundary+*/+real_size=min_t(u64,U32_MAX,+next_stripe_start-cur_disk_byte);+real_size=min_t(u64,real_size,+PAGE_SIZE-offset_in_page(offset));+real_size=min_t(u64,real_size,+compressed_len-offset);+ASSERT(IS_ALIGNED(real_size,fs_info->sectorsize));++added=bio_add_page(comp_bio,page,real_size,+offset_in_page(offset));+/*+*Maximumcompressedextentissmallerthanbiosizelimit,+*thusbio_add_page()shouldalwayssuccess.*/-if(pg_index==nr_pages-1)-pg_len=min_t(u32,PAGE_SIZE,-compressed_len-pg_index*PAGE_SIZE);+ASSERT(added==real_size);+cur_disk_byte+=added;-page=cb->compressed_pages[pg_index];-page->mapping=inode->i_mapping;-page->index=em_start>>PAGE_SHIFT;+/* Reached stripe boundary, need to submit */+if(cur_disk_byte==next_stripe_start)+submit=true;-if(comp_bio->bi_iter.bi_size)-submit=btrfs_bio_fits_in_stripe(page,pg_len,-comp_bio,0);+/* Has finished the range, need to submit */+if(cur_disk_byte==disk_bytenr+compressed_len)+submit=true;-page->mapping=NULL;-if(submit||bio_add_page(comp_bio,page,pg_len,0)<pg_len){+if(submit){unsignedintnr_sectors;ret=btrfs_lookup_bio_sums(inode,comp_bio,sums);
@@ -950,17 +971,18 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,out:free_extent_map(em);returnret;-last_bio:-comp_bio->bi_status=ret;-/* This is the last bio, endio functions will free @cb */-bio_endio(comp_bio);-returnret;finish_cb:if(comp_bio){comp_bio->bi_status=ret;bio_endio(comp_bio);}-wait_var_event(cb,atomic_read(&cb->pending_bios)==0);+/* All bytes of @cb is submitted, endio will free @cb */+if(cur_disk_byte==disk_bytenr+compressed_len)+returnret;++wait_var_event(cb,refcount_read(&cb->pending_sectors)==+(disk_bytenr+compressed_len-cur_disk_byte)>>+fs_info->sectorsize_bits);/**Evenwithpreviousbioended,weshouldstillhaveionotyet*submitted,thusneedtofinish@cbmanually.
From: David Sterba <hidden> Date: 2021-10-04 20:11:16
On Mon, Sep 27, 2021 at 03:21:54PM +0800, Qu Wenruo wrote:
quoted hunk
Currently btrfs_submit_compressed_read() will check
btrfs_bio_fits_in_stripe() each time a new page is going to be added.
Even compressed extent is small, we don't really need to do that for
every page.
This patch will align the behavior to extent_io.c, by determining the
stripe boundary when allocating a bio.
Unlike extent_io.c, in compressed.c we don't need to bother things like
different bio flags, thus no need to re-use bio_ctrl.
Here we just manually introduce new local variable, next_stripe_start,
and teach alloc_compressed_bio() to calculate the stripe boundary.
Then each time we add some page range into the bio, we check if we
reached the boundary.
And if reached, submit it.
Also, since we have @cur_disk_byte to determine whether we're the last
bio, we don't need a explicit last_bio: tag for error handling any more.
And we can use @cur_disk_byte to track which range has been added to
bio, we can also use @cur_disk_byte to calculate the wait condition, no
need for @pending_bios.
Signed-off-by: Qu Wenruo <redacted>
---
fs/btrfs/compression.c | 164 +++++++++++++++++++++++------------------
1 file changed, 93 insertions(+), 71 deletions(-)
Please send an incremental followup to also document all the remaining
parameters. We're not strict about the kdoc warnings but if the code
gets touched it's better to complete it.
*/
static struct bio *alloc_compressed_bio(struct compressed_bio *cb, u64 disk_bytenr,
- unsigned int opf, bio_end_io_t endio_func)
+ unsigned int opf, bio_end_io_t endio_func,
+ u64 *next_stripe_start)
On Mon, Sep 27, 2021 at 03:21:54PM +0800, Qu Wenruo wrote:
quoted
Currently btrfs_submit_compressed_read() will check
btrfs_bio_fits_in_stripe() each time a new page is going to be added.
Even compressed extent is small, we don't really need to do that for
every page.
This patch will align the behavior to extent_io.c, by determining the
stripe boundary when allocating a bio.
Unlike extent_io.c, in compressed.c we don't need to bother things like
different bio flags, thus no need to re-use bio_ctrl.
Here we just manually introduce new local variable, next_stripe_start,
and teach alloc_compressed_bio() to calculate the stripe boundary.
Then each time we add some page range into the bio, we check if we
reached the boundary.
And if reached, submit it.
Also, since we have @cur_disk_byte to determine whether we're the last
bio, we don't need a explicit last_bio: tag for error handling any more.
And we can use @cur_disk_byte to track which range has been added to
bio, we can also use @cur_disk_byte to calculate the wait condition, no
need for @pending_bios.
Signed-off-by: Qu Wenruo <redacted>
---
fs/btrfs/compression.c | 164 +++++++++++++++++++++++------------------
1 file changed, 93 insertions(+), 71 deletions(-)
Please send an incremental followup to also document all the remaining
parameters. We're not strict about the kdoc warnings but if the code
gets touched it's better to complete it.
Currently btrfs_submit_compressed_write() will check
btrfs_bio_fits_in_stripe() each time a new page is going to be added.
Even compressed extent is small, we don't really need to do that for
every page.
This patch will align the behavior to extent_io.c, by determining the
stripe boundary when allocating a bio.
Unlike extent_io.c, in compressed.c we don't need to bother things like
different bio flags, thus no need to re-use bio_ctrl.
Here we just manually introduce new local variable, next_stripe_start,
and use that value returned from alloc_compressed_bio() to calculate
the stripe boundary.
Then each time we add some page range into the bio, we check if we
reached the boundary.
And if reached, submit it.
Also, since we have @cur_disk_bytenr to determine whether we're the last
bio, we don't need a explicit last_bio: tag for error handling any more.
And since we use @cur_disk_bytenr to wait, there is no need for
pending_bios, also remove it to save some memory of compressed_bio.
Signed-off-by: Qu Wenruo <redacted>
---
fs/btrfs/compression.c | 145 ++++++++++++++++++-----------------------
fs/btrfs/compression.h | 3 -
2 files changed, 62 insertions(+), 86 deletions(-)
@@ -527,45 +521,65 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,cb->orig_bio=NULL;cb->nr_pages=nr_pages;-bio=alloc_compressed_bio(cb,first_byte,bio_op|write_flags,-end_compressed_bio_write,-&next_stripe_start);-if(IS_ERR(bio)){-kfree(cb);-returnerrno_to_blk_status(PTR_ERR(bio));-}--if(blkcg_css){-bio->bi_opf|=REQ_CGROUP_PUNT;-kthread_associate_blkcg(blkcg_css);-}--/* create and submit bios for the compressed pages */-bytes_left=compressed_len;-for(pg_index=0;pg_index<cb->nr_pages;pg_index++){-intsubmit=0;-intlen=0;+while(cur_disk_bytenr<disk_start+compressed_len){+u64offset=cur_disk_bytenr-disk_start;+unsignedintindex=offset>>PAGE_SHIFT;+unsignedintreal_size;+unsignedintadded;+structpage*page=compressed_pages[index];+boolsubmit=false;-page=compressed_pages[pg_index];-page->mapping=inode->vfs_inode.i_mapping;-if(bio->bi_iter.bi_size)-submit=btrfs_bio_fits_in_stripe(page,PAGE_SIZE,bio,-0);+/* Allocate new bio if submitted or not yet allocated */+if(!bio){+bio=alloc_compressed_bio(cb,cur_disk_bytenr,+bio_op|write_flags,end_compressed_bio_write,+&next_stripe_start);+if(IS_ERR(bio)){+ret=errno_to_blk_status(PTR_ERR(bio));+bio=NULL;+gotofinish_cb;+}+}+/*+*Weshouldneverreachnext_stripe_startstartaswewill+*submitcomp_biowhenreachtheboundaryimmediately.+*/+ASSERT(cur_disk_bytenr!=next_stripe_start);/*-*Pagecanonlybeaddedtobioifthecurrentbiofitsin-*stripe.+*Wehavevariouslimitontherealreadsize:+*-stripeboundary+*-pageboundary+*-compressedlengthboundary*/-if(!submit){-if(pg_index==0&&use_append)-len=bio_add_zone_append_page(bio,page,-PAGE_SIZE,0);-else-len=bio_add_page(bio,page,PAGE_SIZE,0);-}+real_size=min_t(u64,U32_MAX,+next_stripe_start-cur_disk_bytenr);+real_size=min_t(u64,real_size,+PAGE_SIZE-offset_in_page(offset));+real_size=min_t(u64,real_size,+compressed_len-offset);+ASSERT(IS_ALIGNED(real_size,fs_info->sectorsize));-page->mapping=NULL;-if(submit||len<PAGE_SIZE){+if(use_append)+added=bio_add_zone_append_page(bio,page,real_size,+offset_in_page(offset));+else+added=bio_add_page(bio,page,real_size,+offset_in_page(offset));+/* Reached zoned boundary */+if(added==0)+submit=true;++cur_disk_bytenr+=added;+/* Reached stripe boundary */+if(cur_disk_bytenr==next_stripe_start)+submit=true;++/* Finished the range */+if(cur_disk_bytenr==disk_start+compressed_len)+submit=true;++if(submit){if(!skip_sum){ret=btrfs_csum_one_bio(inode,bio,start,1);if(ret)
@@ -575,61 +589,27 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,ret=submit_compressed_bio(fs_info,cb,bio,0);if(ret)gotofinish_cb;--bio=alloc_compressed_bio(cb,first_byte,-bio_op|write_flags,-end_compressed_bio_write,-&next_stripe_start);-if(IS_ERR(bio)){-ret=errno_to_blk_status(PTR_ERR(bio));-bio=NULL;-gotofinish_cb;-}-if(blkcg_css)-bio->bi_opf|=REQ_CGROUP_PUNT;-/*-*Usebio_add_page()toensurethebiohasatleastone-*page.-*/-bio_add_page(bio,page,PAGE_SIZE,0);-}-if(bytes_left<PAGE_SIZE){-btrfs_info(fs_info,-"bytes left %lu compress len %u nr %u",-bytes_left,cb->compressed_len,cb->nr_pages);+bio=NULL;}-bytes_left-=PAGE_SIZE;-first_byte+=PAGE_SIZE;cond_resched();}--if(!skip_sum){-ret=btrfs_csum_one_bio(inode,bio,start,1);-if(ret)-gotolast_bio;-}--ret=submit_compressed_bio(fs_info,cb,bio,0);-if(ret)-gotolast_bio;-if(blkcg_css)kthread_associate_blkcg(NULL);return0;-last_bio:-bio->bi_status=ret;-/* One of the bios' endio function will free @cb. */-bio_endio(bio);-returnret;finish_cb:if(bio){bio->bi_status=ret;bio_endio(bio);}+/* Last byte of @cb is submitted, endio will free @cb */+if(cur_disk_bytenr==disk_start+compressed_len)+returnret;-wait_var_event(cb,atomic_read(&cb->pending_bios)==0);+wait_var_event(cb,refcount_read(&cb->pending_sectors)==+(disk_start+compressed_len-cur_disk_bytenr)>>+fs_info->sectorsize_bits);/**Evenwithpreviousbioended,weshouldstillhaveionotyet*submitted,thusneedtofinishmanually.
@@ -28,9 +28,6 @@ struct btrfs_inode;#define BTRFS_ZLIB_DEFAULT_LEVEL 3structcompressed_bio{-/* number of bios pending for this compressed extent */-atomic_tpending_bios;-/* Number of sectors with unfinished IO (unsubmitted or unfinished) */refcount_tpending_sectors;
As the last caller in compression.c is removed, we don't need that
function anymore.
Signed-off-by: Qu Wenruo <redacted>
---
fs/btrfs/ctree.h | 2 --
fs/btrfs/inode.c | 42 ------------------------------------------
2 files changed, 44 deletions(-)
We have a big hunk of code inside a while() loop, with tons of strange
jump for error handling.
It's definitely not to the code standard of today.
Move the code into a new function, submit_one_async_extent().
Since we're here, also do the following modifications:
- Comment style change
To follow the current scheme
- Don't fallback to non-compressed write then hitting ENOSPC
If we hit ENOSPC for compressed write, how could we reserve more space
for non-compressed write?
Thus we go error path directly.
This removes the retry: label.
- Add more comment for super long parameter list
Explain which parameter is for, so we don't need to check the
prototype.
- Move the error handling to submit_one_async_extent()
Thus no strange code like:
out_free:
...
goto again;
Signed-off-by: Qu Wenruo <redacted>
---
fs/btrfs/inode.c | 278 +++++++++++++++++++++++------------------------
1 file changed, 137 insertions(+), 141 deletions(-)
@@ -839,163 +839,129 @@ static void free_async_extent_pages(struct async_extent *async_extent)async_extent->pages=NULL;}-/*-*phasetwoofcompressedwriteback.Thisistheorderedportion-*ofthecode,whichonlygetscalledintheordertheworkwas-*queued.Wewalkalltheasyncextentscreatedbycompress_file_range-*andsendthemdowntothedisk.-*/-staticnoinlinevoidsubmit_compressed_extents(structasync_chunk*async_chunk)+staticintsubmit_one_async_extent(structbtrfs_inode*inode,+structasync_chunk*async_chunk,+structasync_extent*async_extent,+u64*alloc_hint){-structbtrfs_inode*inode=BTRFS_I(async_chunk->inode);-structbtrfs_fs_info*fs_info=inode->root->fs_info;-structasync_extent*async_extent;-u64alloc_hint=0;+structextent_io_tree*io_tree=&inode->io_tree;+structbtrfs_root*root=inode->root;+structbtrfs_fs_info*fs_info=root->fs_info;structbtrfs_keyins;structextent_map*em;-structbtrfs_root*root=inode->root;-structextent_io_tree*io_tree=&inode->io_tree;intret=0;+u64start=async_extent->start;+u64end=async_extent->start+async_extent->ram_size-1;-again:-while(!list_empty(&async_chunk->extents)){-async_extent=list_entry(async_chunk->extents.next,-structasync_extent,list);-list_del(&async_extent->list);--retry:-lock_extent(io_tree,async_extent->start,-async_extent->start+async_extent->ram_size-1);-/* did the compression code fall back to uncompressed IO? */-if(!async_extent->pages){-intpage_started=0;-unsignedlongnr_written=0;--/* allocate blocks */-ret=cow_file_range(inode,async_chunk->locked_page,-async_extent->start,-async_extent->start+-async_extent->ram_size-1,-&page_started,&nr_written,0);--/* JDM XXX */+lock_extent(io_tree,start,end);-/*-*ifpage_started,cow_file_rangeinsertedan-*inlineextentandtookcareofalltheunlocking-*andIOforus.Otherwise,weneedtosubmit-*allthosepagesdowntothedrive.-*/-if(!page_started&&!ret)-extent_write_locked_range(&inode->vfs_inode,-async_extent->start,-async_extent->start+-async_extent->ram_size-1,-WB_SYNC_ALL);-elseif(ret&&async_chunk->locked_page)-unlock_page(async_chunk->locked_page);-kfree(async_extent);-cond_resched();-continue;-}--ret=btrfs_reserve_extent(root,async_extent->ram_size,-async_extent->compressed_size,-async_extent->compressed_size,-0,alloc_hint,&ins,1,1);-if(ret){-free_async_extent_pages(async_extent);--if(ret==-ENOSPC){-unlock_extent(io_tree,async_extent->start,-async_extent->start+-async_extent->ram_size-1);--/*-*weneedtoredirtythepagesifwedecideto-*fallbacktouncompressedIO,otherwisewe-*willnotsubmitthesepagesdowntolower-*layers.-*/-extent_range_redirty_for_io(&inode->vfs_inode,-async_extent->start,-async_extent->start+-async_extent->ram_size-1);+/* We have fall back to uncompressed write */+if(!async_extent->pages){+intpage_started=0;+unsignedlongnr_written=0;-gotoretry;-}-gotoout_free;-}/*-*herewe'redoingallocationandwritebackofthe-*compressedpages+*Callcow_file_range()torunthedelallocrangedirectly,+*sincewewon'tgotonocoworasyncpathagain.*/-em=create_io_em(inode,async_extent->start,-async_extent->ram_size,/* len */-async_extent->start,/* orig_start */-ins.objectid,/* block_start */-ins.offset,/* block_len */-ins.offset,/* orig_block_len */-async_extent->ram_size,/* ram_bytes */-async_extent->compress_type,-BTRFS_ORDERED_COMPRESSED);-if(IS_ERR(em))-/* ret value is not necessary due to void function */-gotoout_free_reserve;-free_extent_map(em);--ret=btrfs_add_ordered_extent_compress(inode,-async_extent->start,-ins.objectid,-async_extent->ram_size,-ins.offset,-async_extent->compress_type);-if(ret){-btrfs_drop_extent_cache(inode,async_extent->start,-async_extent->start+-async_extent->ram_size-1,0);-gotoout_free_reserve;-}-btrfs_dec_block_group_reservations(fs_info,ins.objectid);-+ret=cow_file_range(inode,async_chunk->locked_page,+start,end,&page_started,&nr_written,0);/*-*cleardirty,setwritebackandunlockthepages.+*If@page_started,cow_file_range()insertedan+*inlineextentandtookcareofalltheunlocking+*andIOforus.Otherwise,weneedtosubmit+*allthosepagesdowntothedrive.*/-extent_clear_unlock_delalloc(inode,async_extent->start,-async_extent->start+-async_extent->ram_size-1,-NULL,EXTENT_LOCKED|EXTENT_DELALLOC,-PAGE_UNLOCK|PAGE_START_WRITEBACK);-if(btrfs_submit_compressed_write(inode,async_extent->start,-async_extent->ram_size,-ins.objectid,-ins.offset,async_extent->pages,-async_extent->nr_pages,-async_chunk->write_flags,-async_chunk->blkcg_css)){-constu64start=async_extent->start;-constu64end=start+async_extent->ram_size-1;--btrfs_writepage_endio_finish_ordered(inode,NULL,start,-end,false);--extent_clear_unlock_delalloc(inode,start,end,NULL,0,-PAGE_END_WRITEBACK|-PAGE_SET_ERROR);-free_async_extent_pages(async_extent);-}-alloc_hint=ins.objectid+ins.offset;+if(!page_started&&!ret)+extent_write_locked_range(&inode->vfs_inode,start,+end,WB_SYNC_ALL);+elseif(ret&&async_chunk->locked_page)+unlock_page(async_chunk->locked_page);kfree(async_extent);-cond_resched();+returnret;+}++ret=btrfs_reserve_extent(root,async_extent->ram_size,+async_extent->compressed_size,+async_extent->compressed_size,+0,*alloc_hint,&ins,1,1);+if(ret){+free_async_extent_pages(async_extent);+/*+*Hereweusedtotryagainbygoingbacktonon-compressed+*pathforENOSPC.+*Butwecan'treservespaceevenforcompressedsize,how+*coulditworkforuncompressedsizewhichrequireslarger+*size?+*Soherewedirectlygoerrorpath.+*/+gotoout_free;+}++/*+*Herewe'redoingallocationandwritebackofthe+*compressedpages+*/+em=create_io_em(inode,start,+async_extent->ram_size,/* len */+start,/* orig_start */+ins.objectid,/* block_start */+ins.offset,/* block_len */+ins.offset,/* orig_block_len */+async_extent->ram_size,/* ram_bytes */+async_extent->compress_type,+BTRFS_ORDERED_COMPRESSED);+if(IS_ERR(em)){+ret=PTR_ERR(em);+gotoout_free_reserve;+}+free_extent_map(em);++ret=btrfs_add_ordered_extent_compress(inode,start,/* file_offset */+ins.objectid,/* disk_bytenr */+async_extent->ram_size,/* num_bytes */+ins.offset,/* disk_num_bytes */+async_extent->compress_type);+if(ret){+btrfs_drop_extent_cache(inode,start,end,0);+gotoout_free_reserve;}-return;+btrfs_dec_block_group_reservations(fs_info,ins.objectid);++/*+*cleardirty,setwritebackandunlockthepages.+*/+extent_clear_unlock_delalloc(inode,start,end,+NULL,EXTENT_LOCKED|EXTENT_DELALLOC,+PAGE_UNLOCK|PAGE_START_WRITEBACK);+if(btrfs_submit_compressed_write(inode,start,/* file_offset */+async_extent->ram_size,/* num_bytes */+ins.objectid,/* disk_bytenr */+ins.offset,/* compressed_len */+async_extent->pages,/* compressed_pages */+async_extent->nr_pages,+async_chunk->write_flags,+async_chunk->blkcg_css)){+constu64start=async_extent->start;+constu64end=start+async_extent->ram_size-1;++btrfs_writepage_endio_finish_ordered(inode,NULL,start,+end,0);++extent_clear_unlock_delalloc(inode,start,end,NULL,0,+PAGE_END_WRITEBACK|+PAGE_SET_ERROR);+free_async_extent_pages(async_extent);+}+*alloc_hint=ins.objectid+ins.offset;+kfree(async_extent);+returnret;+out_free_reserve:btrfs_dec_block_group_reservations(fs_info,ins.objectid);btrfs_free_reserved_extent(fs_info,ins.objectid,ins.offset,1);out_free:-extent_clear_unlock_delalloc(inode,async_extent->start,-async_extent->start+-async_extent->ram_size-1,+extent_clear_unlock_delalloc(inode,start,end,NULL,EXTENT_LOCKED|EXTENT_DELALLOC|EXTENT_DELALLOC_NEW|EXTENT_DEFRAG|EXTENT_DO_ACCOUNTING,
From: David Sterba <hidden> Date: 2021-10-05 16:33:53
On Mon, Sep 27, 2021 at 03:21:57PM +0800, Qu Wenruo wrote:
We have a big hunk of code inside a while() loop, with tons of strange
jump for error handling.
It's definitely not to the code standard of today.
Move the code into a new function, submit_one_async_extent().
Since we're here, also do the following modifications:
- Comment style change
To follow the current scheme
- Don't fallback to non-compressed write then hitting ENOSPC
If we hit ENOSPC for compressed write, how could we reserve more space
for non-compressed write?
Thus we go error path directly.
This removes the retry: label.
I'm not happy about mixing this change with a refactoring, as it's a
functional change and in the logic how compressed writes do the
fallback. This should have proper reasoning and some kind of independent
testing. As it does not sound wrong I'll leave as it is, hopefully this
won't haunt us in the future.
There are several cleanups for extent_write_locked_range(), most of them
are pure cleanups, but with some preparation for future subpage support.
- Add a proper comment for which call sites are suitable
Unlike regular synchronized extent write back, if async cow or zoned
cow happens, we have all pages in the range still locked.
Thus for those (only) two call sites, we need this function to submit
page content into bios and submit them.
- Remove @mode parameter
All the existing two call sites pass WB_SYNC_ALL. No need for @mode
parameter.
- Better error handling
Currently if we hit an error during the page iteration loop, we
overwrite @ret, causing only the last error can be recorded.
Here we add @found_error and @first_error variable to record if we hit
any error, and the first error we hit.
So the first error won't get lost.
- Don't reuse @start as the cursor
We reuse the parameter @start as the cursor to iterate the range, not
a big problem, but since we're here, introduce a proper @cur as the
cursor.
- Remove impossible branch
Since all pages are still locked after the ordered extent is inserted,
there is no way that pages can get its dirty bit cleared.
Remove the branch where page is not dirty and replace it with an
ASSERT().
Signed-off-by: Qu Wenruo <redacted>
---
fs/btrfs/extent_io.c | 45 ++++++++++++++++++++++++++++----------------
fs/btrfs/extent_io.h | 3 +--
fs/btrfs/inode.c | 4 ++--
3 files changed, 32 insertions(+), 20 deletions(-)
@@ -5074,23 +5074,29 @@ int extent_write_full_page(struct page *page, struct writeback_control *wbc)returnret;}-intextent_write_locked_range(structinode*inode,u64start,u64end,-intmode)+/*+*Submitthepagesintherangetobioforcallsiteswhichdelallocrange+*hasalreadyberan(aka,orderedextentinserted)andallpagesarestill+*locked.+*/+intextent_write_locked_range(structinode*inode,u64start,u64end){+boolfound_error=false;+intfirst_error=0;intret=0;structaddress_space*mapping=inode->i_mapping;structpage*page;+u64cur=start;unsignedlongnr_pages=(end-start+PAGE_SIZE)>>PAGE_SHIFT;-structextent_page_dataepd={.bio_ctrl={0},.extent_locked=1,-.sync_io=mode==WB_SYNC_ALL,+.sync_io=1,};structwriteback_controlwbc_writepages={-.sync_mode=mode,.nr_to_write=nr_pages*2,+.sync_mode=WB_SYNC_ALL,.range_start=start,.range_end=end+1,/* We're called from an async helper function */
In function compress_file_range(), when the compression is finished, the
function just round up @total_in to PAGE_SIZE.
This is fine for regular sectorsize == PAGE_SIZE case, but not for
subpage.
Just change the ALIGN(, PAGE_SIZE) to round_up(, sectorsize) so that
both regular sectorsize and subpage sectorsize will be happy.
Signed-off-by: Qu Wenruo <redacted>
---
fs/btrfs/inode.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
There is a WARN_ON() checking if @start is aligned to PAGE_SIZE, not
sectorsize, which will cause false alert for subpage.
Fix it to check against sectorsize.
Furthermore:
- Use ASSERT() to do the check
So that in the future we may skip the check for production build
- Also check alignment for @len
Signed-off-by: Qu Wenruo <redacted>
---
fs/btrfs/compression.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
In end_compressed_writeback() we just clear the full page writeback.
For subpage case, if there are two delalloc range in the same page, the
2nd range will trigger a BUG_ON() as the page writeback is already
cleared by previous range.
Fix it by using btrfs_page_clamp_clear_writeback() helper.
Signed-off-by: Qu Wenruo <redacted>
---
fs/btrfs/compression.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
There are two sites are not subpage compatible yet for
extent_write_locked_range():
- How @nr_pages are calculated
For subpage we can have the following range with 64K page size:
0 32K 64K 96K 128K
| |////|/////| |
In that case, although 96K - 32K == 64K, thus it looks like one page
is enough, but the range spans across two pages, not one.
Fix it by doing proper round_up() and round_down() to calculate
@nr_pages.
Also add some extra ASSERT()s to ensure the range passed in is already
aligned.
- How the page end is calculated
Currently we just use cur + PAGE_SIZE - 1 to calculate the page end.
Which can't handle above range layout, and will trigger ASSERT() in
btrfs_writepage_endio_finish_ordered(), as the range is no longer
covered by the page range.
Fix it by take page end into consideration.
Signed-off-by: Qu Wenruo <redacted>
---
fs/btrfs/extent_io.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
From: Josef Bacik <josef@toxicpanda.com> Date: 2021-10-13 14:50:43
On Mon, Sep 27, 2021 at 03:22:02PM +0800, Qu Wenruo wrote:
quoted hunk
There are two sites are not subpage compatible yet for
extent_write_locked_range():
- How @nr_pages are calculated
For subpage we can have the following range with 64K page size:
0 32K 64K 96K 128K
| |////|/////| |
In that case, although 96K - 32K == 64K, thus it looks like one page
is enough, but the range spans across two pages, not one.
Fix it by doing proper round_up() and round_down() to calculate
@nr_pages.
Also add some extra ASSERT()s to ensure the range passed in is already
aligned.
- How the page end is calculated
Currently we just use cur + PAGE_SIZE - 1 to calculate the page end.
Which can't handle above range layout, and will trigger ASSERT() in
btrfs_writepage_endio_finish_ordered(), as the range is no longer
covered by the page range.
Fix it by take page end into consideration.
Signed-off-by: Qu Wenruo <redacted>
---
fs/btrfs/extent_io.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
We're tripping this ASSERT() with compression turned on, sorry I didn't notice
this but we've been panicing consistently since this was merged, so I've lost a
weeks worth of xfstests runs. You can easily reproduce running
./check generic/029
with
MOUNT_OPTIONS="-o compress"
Thanks,
Josef
On Mon, Sep 27, 2021 at 03:22:02PM +0800, Qu Wenruo wrote:
quoted
There are two sites are not subpage compatible yet for
extent_write_locked_range():
- How @nr_pages are calculated
For subpage we can have the following range with 64K page size:
0 32K 64K 96K 128K
| |////|/////| |
In that case, although 96K - 32K == 64K, thus it looks like one page
is enough, but the range spans across two pages, not one.
Fix it by doing proper round_up() and round_down() to calculate
@nr_pages.
Also add some extra ASSERT()s to ensure the range passed in is already
aligned.
- How the page end is calculated
Currently we just use cur + PAGE_SIZE - 1 to calculate the page end.
Which can't handle above range layout, and will trigger ASSERT() in
btrfs_writepage_endio_finish_ordered(), as the range is no longer
covered by the page range.
Fix it by take page end into consideration.
Signed-off-by: Qu Wenruo <redacted>
---
fs/btrfs/extent_io.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
We're tripping this ASSERT() with compression turned on, sorry I didn't notice
this but we've been panicing consistently since this was merged, so I've lost a
weeks worth of xfstests runs. You can easily reproduce running
./check generic/029
with
MOUNT_OPTIONS="-o compress"
Confirmed, but this also means, some pages are no longer locked for
compression.
This doesn't sound correct to me, will do more investigation to find out
why.
Thanks,
Qu
Introduce a new helper, submit_uncompressed_range(), for async cow cases
where we fallback to cow.
There are some new modification introduced to the helper:
- Proper locked_page detection
It's possible that the async_extent range doesn't cover the locked
page.
In that case we shouldn't unlock the locked page.
In the new helper, we will ensure that we only unlock the locked page
when:
* The locked page covers part of the async_extent range
* The locked page is not unlocked by cow_file_range() nor
extent_write_locked_range()
This also means extra comments are added focusing on the page locking.
- Add extra comment on some rare parameter used.
We use @unlock_page = 0 for cow_file_range(), where only two call
sites doing the same thing, including the new helper.
It's definitely worthy some comments.
Signed-off-by: Qu Wenruo <redacted>
---
fs/btrfs/inode.c | 76 +++++++++++++++++++++++++++++++++---------------
1 file changed, 52 insertions(+), 24 deletions(-)
@@ -839,6 +839,43 @@ static void free_async_extent_pages(struct async_extent *async_extent)async_extent->pages=NULL;}+staticintsubmit_uncompressed_range(structbtrfs_inode*inode,+structasync_extent*async_extent,+structpage*locked_page)+{+u64start=async_extent->start;+u64end=async_extent->start+async_extent->ram_size-1;+unsignedlongnr_written=0;+intpage_started=0;+intret;++/*+*Callcow_file_range()torunthedelallocrangedirectly,+*sincewewon'tgotonocoworasyncpathagain.+*+*Alsowecallcow_file_range()with@unlock_page==0,sothatwe+*candirectlysubmitthemwithoutinterruption.+*/+ret=cow_file_range(inode,locked_page,start,end,&page_started,+&nr_written,0);+/* Inline extent inserted, page get unlocked and everything is done */+if(ret>0){+ret=0;+gotoout;+}+if(ret<0){+if(locked_page)+unlock_page(locked_page);+gotoout;+}++ret=extent_write_locked_range(&inode->vfs_inode,start,end);+/* All pages will be unlocked, including @locked_page */+out:+kfree(async_extent);+returnret;+}+staticintsubmit_one_async_extent(structbtrfs_inode*inode,structasync_chunk*async_chunk,structasync_extent*async_extent,
@@ -848,38 +885,29 @@ static int submit_one_async_extent(struct btrfs_inode *inode,structbtrfs_root*root=inode->root;structbtrfs_fs_info*fs_info=root->fs_info;structbtrfs_keyins;+structpage*locked_page=NULL;structextent_map*em;intret=0;u64start=async_extent->start;u64end=async_extent->start+async_extent->ram_size-1;+/*+*Ifasync_chunk->locked_pageisintheasync_extentrange,we+*needtohandleit.+*/+if(async_chunk->locked_page){+u64locked_page_start=page_offset(async_chunk->locked_page);+u64locked_page_end=locked_page_start+PAGE_SIZE-1;++if(!(start>=locked_page_end||end<=locked_page_start))+locked_page=async_chunk->locked_page;+}lock_extent(io_tree,start,end);/* We have fall back to uncompressed write */-if(!async_extent->pages){-intpage_started=0;-unsignedlongnr_written=0;--/*-*Callcow_file_range()torunthedelallocrangedirectly,-*sincewewon'tgotonocoworasyncpathagain.-*/-ret=cow_file_range(inode,async_chunk->locked_page,-start,end,&page_started,&nr_written,0);-/*-*If@page_started,cow_file_range()insertedan-*inlineextentandtookcareofalltheunlocking-*andIOforus.Otherwise,weneedtosubmit-*allthosepagesdowntothedrive.-*/-if(!page_started&&!ret)-extent_write_locked_range(&inode->vfs_inode,start,-end);-elseif(ret&&async_chunk->locked_page)-unlock_page(async_chunk->locked_page);-kfree(async_extent);-returnret;-}+if(!async_extent->pages)+returnsubmit_uncompressed_range(inode,async_extent,+locked_page);ret=btrfs_reserve_extent(root,async_extent->ram_size,async_extent->compressed_size,
There are several problems in lzo_compress_pages() preventing it from
being subpage compatible:
- No page offset is calculated when reading from inode pages
For subpage case, we could have @start which is not aligned to
PAGE_SIZE.
Thus the destination where we read data from must take offset in page
into consideration.
- The padding for segment header is bound to PAGE_SIZE
This means, for subpage case we can skip several corners where on x86
machines we need to add padding zeros.
The rework will:
- Update the comment to replace "page" with "sector"
- Introduce a new helper, copy_compressed_data_to_page(), to do the copy
So that we don't need to bother page switches for both input and
output.
Now in lzo_compress_pages() we only care about page switching for
input, while in copy_compressed_data_to_page() we only care the page
switching for output.
- Only one main cursor
For lzo_compress_pages() we use @cur_in as main curor.
It will be the file offset we are currently at.
All other helper variables will be only declared inside the loop.
For copy_compressed_data_to_page() it's similar, we will have
@cur_out at the main cursor, which records how many bytes are in the
output.
- Get rid of kmap()/kunmap()
Instead of using __GFP_HIGHMEM and needs to do kmap()/kunmap(), just
get rid of that GFP flag, so we can use page_address() and never
bother the kmap()/kunmap() thing.
Signed-off-by: Qu Wenruo <redacted>
---
fs/btrfs/lzo.c | 270 ++++++++++++++++++++++++-------------------------
1 file changed, 134 insertions(+), 136 deletions(-)
@@ -112,163 +112,161 @@ static inline size_t read_compress_length(const char *buf)returnle32_to_cpu(dlen);}+/*+*Willdo:+*+*-Writeasegmentheaderintothedestination+*-Copythecompressedbufferintothedestination+*-Makesurewehaveenoughspaceinthelastsectortofitasegmentheader+*Ifnot,wewillpadatmost(LZO_LEN(4))-1bytesofzeros.+*+*Willallocatenewpageswhenneeded.+*/+staticintcopy_compressed_data_to_page(char*compressed_data,+size_tcompressed_size,+structpage**out_pages,+u32*cur_out,+constu32sectorsize)+{+u32sector_bytes_left;+u32orig_out;+structpage*cur_page;++/*+*Weneverallowasegmentheadercrossingsectorboundary,previous+*runshouldensurewehaveenoughspaceleftinsidethesector.+*/+ASSERT((*cur_out/sectorsize)==+(*cur_out+LZO_LEN-1)/sectorsize);++cur_page=out_pages[*cur_out/PAGE_SIZE];+/* Allocate a new page */+if(!cur_page){+cur_page=alloc_page(GFP_NOFS);+if(!cur_page)+return-ENOMEM;+out_pages[*cur_out/PAGE_SIZE]=cur_page;+}++write_compress_length(page_address(cur_page)+offset_in_page(*cur_out),+compressed_size);+*cur_out+=LZO_LEN;++orig_out=*cur_out;++/* Copy compressed data */+while(*cur_out-orig_out<compressed_size){+u32copy_len=min_t(u32,sectorsize-*cur_out%sectorsize,+orig_out+compressed_size-*cur_out);++cur_page=out_pages[*cur_out/PAGE_SIZE];+/* Allocate a new page */+if(!cur_page){+cur_page=alloc_page(GFP_NOFS);+if(!cur_page)+return-ENOMEM;+out_pages[*cur_out/PAGE_SIZE]=cur_page;+}++memcpy(page_address(cur_page)+offset_in_page(*cur_out),+compressed_data+*cur_out-orig_out,copy_len);++*cur_out+=copy_len;+}++/*+*Checkifwecanfitthenextsegmentheaderintotheremainingspace+*ofthesector.+*/+sector_bytes_left=round_up(*cur_out,sectorsize)-*cur_out;+if(sector_bytes_left>=LZO_LEN||sector_bytes_left==0)+return0;++/* The remaining size is not enough, pad it with zeros */+memset(page_address(cur_page)+offset_in_page(*cur_out),0,+sector_bytes_left);+*cur_out+=sector_bytes_left;+return0;+}+intlzo_compress_pages(structlist_head*ws,structaddress_space*mapping,u64start,structpage**pages,unsignedlong*out_pages,unsignedlong*total_in,unsignedlong*total_out){structworkspace*workspace=list_entry(ws,structworkspace,list);+constu32sectorsize=btrfs_sb(mapping->host->i_sb)->sectorsize;+structpage*page_in=NULL;intret=0;-char*data_in;-char*cpage_out,*sizes_ptr;-intnr_pages=0;-structpage*in_page=NULL;-structpage*out_page=NULL;-unsignedlongbytes_left;-unsignedlonglen=*total_out;-unsignedlongnr_dest_pages=*out_pages;-constunsignedlongmax_out=nr_dest_pages*PAGE_SIZE;-size_tin_len;-size_tout_len;-char*buf;-unsignedlongtot_in=0;-unsignedlongtot_out=0;-unsignedlongpg_bytes_left;-unsignedlongout_offset;-unsignedlongbytes;+u64cur_in=start;/* Points to the file offset of input data */+u32cur_out=0;/* Points to the current output byte */+u32len=*total_out;*out_pages=0;*total_out=0;*total_in=0;-in_page=find_get_page(mapping,start>>PAGE_SHIFT);-data_in=page_address(in_page);-/*-*storethesizeofallchunksofcompresseddatain-*thefirst4bytes+*Skiptheheaderfornow,wewilllatercomebackandwritethetotal+*compressedsize*/-out_page=alloc_page(GFP_NOFS);-if(out_page==NULL){-ret=-ENOMEM;-gotoout;-}-cpage_out=page_address(out_page);-out_offset=LZO_LEN;-tot_out=LZO_LEN;-pages[0]=out_page;-nr_pages=1;-pg_bytes_left=PAGE_SIZE-LZO_LEN;--/* compress at most one page of data each time */-in_len=min(len,PAGE_SIZE);-while(tot_in<len){-ret=lzo1x_1_compress(data_in,in_len,workspace->cbuf,-&out_len,workspace->mem);-if(ret!=LZO_E_OK){-pr_debug("BTRFS: lzo in loop returned %d\n",-ret);+cur_out+=LZO_LEN;+while(cur_in<start+len){+u32sector_off=(cur_in-start)%sectorsize;+u32in_len;+size_tout_len;++/* Get the input page first */+if(!page_in){+page_in=find_get_page(mapping,cur_in>>PAGE_SHIFT);+ASSERT(page_in);+}++/* Compress at most one sector of data each time */+in_len=min_t(u32,start+len-cur_in,+sectorsize-sector_off);+ASSERT(in_len);+ret=lzo1x_1_compress(page_address(page_in)++offset_in_page(cur_in),in_len,+workspace->cbuf,&out_len,+workspace->mem);+if(ret<0){+pr_debug("BTRFS: lzo in loop returned %d\n",ret);ret=-EIO;gotoout;}-/* store the size of this chunk of compressed data */-write_compress_length(cpage_out+out_offset,out_len);-tot_out+=LZO_LEN;-out_offset+=LZO_LEN;-pg_bytes_left-=LZO_LEN;--tot_in+=in_len;-tot_out+=out_len;--/* copy bytes from the working buffer into the pages */-buf=workspace->cbuf;-while(out_len){-bytes=min_t(unsignedlong,pg_bytes_left,out_len);--memcpy(cpage_out+out_offset,buf,bytes);--out_len-=bytes;-pg_bytes_left-=bytes;-buf+=bytes;-out_offset+=bytes;--/*-*weneedanotherpageforwritingout.-*-*Noteifthere'slessthan4bytesleft,wejust-*skiptoanewpage.-*/-if((out_len==0&&pg_bytes_left<LZO_LEN)||-pg_bytes_left==0){-if(pg_bytes_left){-memset(cpage_out+out_offset,0,-pg_bytes_left);-tot_out+=pg_bytes_left;-}--/* we're done, don't allocate new page */-if(out_len==0&&tot_in>=len)-break;--if(nr_pages==nr_dest_pages){-out_page=NULL;-ret=-E2BIG;-gotoout;-}--out_page=alloc_page(GFP_NOFS);-if(out_page==NULL){-ret=-ENOMEM;-gotoout;-}-cpage_out=page_address(out_page);-pages[nr_pages++]=out_page;--pg_bytes_left=PAGE_SIZE;-out_offset=0;-}-}+ret=copy_compressed_data_to_page(workspace->cbuf,out_len,+pages,&cur_out,sectorsize);+if(ret<0)+gotoout;++cur_in+=in_len;-/* we're making it bigger, give up */-if(tot_in>8192&&tot_in<tot_out){+/*+*Checkifwe'remakingitbiggeraftertwosectors.+*Andifwe'remakingitbigger,giveup.+*/+if(cur_in-start>sectorsize*2&&+cur_in-start<cur_out){ret=-E2BIG;gotoout;}-/* we're all done */-if(tot_in>=len)-break;--if(tot_out>max_out)-break;--bytes_left=len-tot_in;-put_page(in_page);--start+=PAGE_SIZE;-in_page=find_get_page(mapping,start>>PAGE_SHIFT);-data_in=page_address(in_page);-in_len=min(bytes_left,PAGE_SIZE);-}--if(tot_out>=tot_in){-ret=-E2BIG;-gotoout;+/* Check if we have reached page boundary */+if(IS_ALIGNED(cur_in,PAGE_SIZE)){+put_page(page_in);+page_in=NULL;+}}-/* store the size of all chunks of compressed data */-sizes_ptr=page_address(pages[0]);-write_compress_length(sizes_ptr,tot_out);+/* Store the size of all chunks of compressed data */+write_compress_length(page_address(pages[0]),cur_out);ret=0;-*total_out=tot_out;-*total_in=tot_in;+*total_out=cur_out;+*total_in=cur_in-start;out:-*out_pages=nr_pages;--if(in_page)-put_page(in_page);-+*out_pages=DIV_ROUND_UP(cur_out,PAGE_SIZE);returnret;}
Pages passed to __extent_writepage() are always locked, but they may be
locked by different functions.
There are two types of locked page for __extent_writepage():
- Page locked by plain lock_page()
It should not have any subpage::writers count.
Can be unlocked by unlock_page().
This is the most common locked page for __extent_writepage() called
inside extent_write_cache_pages() or extent_write_full_page().
Rarer cases includes the @locked_page from extent_write_locked_range().
- Page locked by lock_delalloc_pages()
There is only one caller, all pages except @locked_page for
extent_write_locked_range().
In this case, we have to call subpage helper to handle the case.
So here we introduce a helper, btrfs_page_unlock_writer(), to allow
__extent_writepage() to unlock different locked pages.
And since for all other callers of __extent_writepage() their pages are
ensured to be locked by lock_page(), also add an extra check for
epd::extent_locked to unlock such pages directly.
Signed-off-by: Qu Wenruo <redacted>
---
fs/btrfs/extent_io.c | 15 ++++++++++++++-
fs/btrfs/subpage.c | 43 +++++++++++++++++++++++++++++++++++++++++++
fs/btrfs/subpage.h | 2 ++
3 files changed, 59 insertions(+), 1 deletion(-)
@@ -691,3 +691,46 @@ void btrfs_page_assert_not_dirty(const struct btrfs_fs_info *fs_info,ASSERT(PagePrivate(page)&&page->private);ASSERT(subpage_test_bitmap_all_zero(fs_info,subpage,dirty));}++/*+*Helpertohandledifferentlockedpagewithdifferentpagesize+*-Pagelockedbyplainlock_page()+*Itshouldnothaveanysubpage::writerscount.+*Canbeunlockedbyunlock_page().+*Thisisthemostcommonlockedpagefor__extent_writepage()called+*insideextent_write_cache_pages()orextent_write_full_page().+*Rarercasesincludesthe@locked_pagefromextent_write_locked_range().+*+*-Pagelockedbylock_delalloc_pages()+*Thereisonlyonecaller,allpagesexcept@locked_pagefor+*extent_write_locked_range().+*Inthiscase,wehavetocallsubpagehelpertohandlethecase.+*/+voidbtrfs_page_unlock_writer(structbtrfs_fs_info*fs_info,structpage*page,+u64start,u32len)+{+structbtrfs_subpage*subpage;++ASSERT(PageLocked(page));+/* For regular page size case, we just unlock the page */+if(fs_info->sectorsize==PAGE_SIZE)+returnunlock_page(page);++ASSERT(PagePrivate(page)&&page->private);+subpage=(structbtrfs_subpage*)page->private;++/*+*Forsubpagecase,therearetwotypesoflockedpage.+*Withorwithoutwritersnumber.+*+*Sinceweownthepagelock,nooneelsecouldtouch+*subpage::writersandarewesafetodoseveralatomicoperations+*withoutspinlock.+*/+if(atomic_read(&subpage->writers))+/* No writers, locked by plain lock_page() */+returnunlock_page(page);++/* Have writers, use proper subpage helper to end it */+btrfs_page_end_writer_lock(fs_info,page,start,len);+}
There are several call sites of extent_clear_unlock_delalloc() which
gets @locked_page = NULL.
So that extent_clear_unlock_delalloc() will try to call
process_one_page() to unlock every page even the first page is not
locked by btrfs_page_start_writer_lock().
This will trigger an ASSERT() in btrfs_subpage_end_and_test_writer() as
previously we require every page passed to
btrfs_subpage_end_and_test_writer() to be locked by
btrfs_page_start_writer_lock().
But compression path doesn't go that way.
Thankfully it's not hard to distinguish page locked by lock_page() and
btrfs_page_start_writer_lock().
So do the check in btrfs_subpage_end_and_test_writer() so now it can
handle both cases well.
Signed-off-by: Qu Wenruo <redacted>
---
fs/btrfs/subpage.c | 11 +++++++++++
1 file changed, 11 insertions(+)
[BUG]
With experimental subpage compression enabled, a simple fsstress can
lead to self deadlock on page 720896:
mkfs.btrfs -f -s 4k $dev > /dev/null
mount $dev -o compress $mnt
$fsstress -p 1 -n 100 -w -d $mnt -v -s 1625511156
[CAUSE]
If we have a file layout looks like below:
0 32K 64K 96K 128K
|//| |///////////////|
4K
Then we run delalloc range for the inode, it will:
- Call find_lock_delalloc_range() with @delalloc_start = 0
Then we got a delalloc range [0, 4K).
This range will be CoWed.
- Call find_lock_delalloc_range() again with @delalloc_start = 4K
Since find_lock_delalloc_range() never cares whether the range
is still inside page range [0, 64K), it will return range [64K, 128K).
This range meets the condition for subpage compression, will go
through async cow path.
And async cow path will return @page_started.
But that @page_started is now for range [64K, 128K), not for range
[0, 64K).
- writepage_dellloc() returned 1 for page [0, 64K)
Thus page [0, 64K) will not be unlocked, nor its page dirty status
will be cleared.
Next time when we try to lock page [0, 64K) we will deadlock, as there
is no one to release page [0, 64K).
This problem will never happen for regular page size as one page only
contains one sector.
After the first find_lock_delalloc_range() call, the @delalloc_end will
go beyond @page_end no matter if we found a delalloc range or not
Thus this bug only happens for subpage, as now we need multiple runs to
exhaust the delalloc range of a page.
[FIX]
Fix the problem by ensure the delalloc range we ran at least starts
inside @locked_page.
So that we will never got incorrect @page_started.
And to prevent such problem from happening again:
- Make find_lock_delalloc_range() to return false if the found range is
beyond @end value passed in.
Since @end will be utilized now, add an ASSERT() to ensure we pass
correct @end into find_lock_delalloc_range().
This also means, for selftest we needs to populate @end before calling
find_lock_delalloc_range().
- New ASSERT() in find_lock_delalloc_range()
Now we will make sure the @start/@end passed in at least covers part
of the page.
- New ASSERT() in run_delalloc_range()
To make sure the range at least starts inside @locked page.
- Use @delalloc_start as proper cursor, while @delalloc_end is always
reset to @page_end.
Signed-off-by: Qu Wenruo <redacted>
---
fs/btrfs/extent_io.c | 36 ++++++++++++++++++++++++--------
fs/btrfs/inode.c | 7 +++++++
fs/btrfs/tests/extent-io-tests.c | 12 +++++------
3 files changed, 40 insertions(+), 15 deletions(-)
@@ -1994,15 +2004,23 @@ noinline_for_stack bool find_lock_delalloc_range(struct inode *inode,intret;intloops=0;+/* Caller should pass a valid @end to indicate the search range end */+ASSERT(orig_end>orig_start);++/* The range should at least cover part of the page */+ASSERT(!(orig_start>=page_offset(locked_page)+PAGE_SIZE||+orig_end<=page_offset(locked_page)));again:/* step one, find a bunch of delalloc bytes starting at start */delalloc_start=*start;delalloc_end=0;found=btrfs_find_delalloc_range(tree,&delalloc_start,&delalloc_end,max_bytes,&cached_state);-if(!found||delalloc_end<=*start){+if(!found||delalloc_end<=*start||delalloc_start>orig_end){*start=delalloc_start;-*end=delalloc_end;++/* @delalloc_end can be -1, never go beyond @orig_end */+*end=min(delalloc_end,orig_end);free_extent_state(cached_state);returnfalse;}
@@ -3771,16 +3789,16 @@ static noinline_for_stack int writepage_delalloc(struct btrfs_inode *inode,structpage*page,structwriteback_control*wbc,unsignedlong*nr_written){-u64page_end=page_offset(page)+PAGE_SIZE-1;-boolfound;+constu64page_end=page_offset(page)+PAGE_SIZE-1;u64delalloc_start=page_offset(page);u64delalloc_to_write=0;-u64delalloc_end=0;intret;intpage_started=0;+while(delalloc_start<page_end){+u64delalloc_end=page_end;+boolfound;-while(delalloc_end<page_end){found=find_lock_delalloc_range(&inode->vfs_inode,page,&delalloc_start,&delalloc_end);
@@ -112,7 +112,7 @@ static int test_find_delalloc(u32 sectorsize)*/set_extent_delalloc(tmp,0,sectorsize-1,0,NULL);start=0;-end=0;+end=start+PAGE_SIZE-1;found=find_lock_delalloc_range(inode,locked_page,&start,&end);if(!found){
@@ -143,7 +143,7 @@ static int test_find_delalloc(u32 sectorsize)}set_extent_delalloc(tmp,sectorsize,max_bytes-1,0,NULL);start=test_start;-end=0;+end=start+PAGE_SIZE-1;found=find_lock_delalloc_range(inode,locked_page,&start,&end);if(!found){
@@ -177,14 +177,14 @@ static int test_find_delalloc(u32 sectorsize)gotoout_bits;}start=test_start;-end=0;+end=start+PAGE_SIZE-1;found=find_lock_delalloc_range(inode,locked_page,&start,&end);if(found){test_err("found range when we shouldn't have");gotoout_bits;}-if(end!=(u64)-1){+if(end!=test_start+PAGE_SIZE-1){test_err("did not return the proper end offset");gotoout_bits;}
@@ -198,7 +198,7 @@ static int test_find_delalloc(u32 sectorsize)*/set_extent_delalloc(tmp,max_bytes,total_dirty-1,0,NULL);start=test_start;-end=0;+end=start+PAGE_SIZE-1;found=find_lock_delalloc_range(inode,locked_page,&start,&end);if(!found){
@@ -233,7 +233,7 @@ static int test_find_delalloc(u32 sectorsize)/* We unlocked it in the previous test */lock_page(locked_page);start=test_start;-end=0;+end=start+PAGE_SIZE-1;/**Currentlyifwefailtofinddirtypagesinthedelallocrangewe*willadjustmax_bytesdowntoPAGE_SIZEandthenre-search.If
For btrfs compressed write, we use a mechanism called async cow, which
unlike regular run_delalloc_cow() or cow_file_range(), it will also
unlock the first page.
This mechanism allows btrfs to continue handling next ranges, without
waiting for the time consuming compression.
But this has a problem for subpage case, as we could have the following
delalloc range for a page:
0 32K 64K
| |///////| |///////|
\- A \- B
In above case, if we pass both range to cow_file_range_async(), both
range A and range B will try to unlock the full page [0, 64K).
And which finishes later than the other range will try to do other page
operations like end_page_writeback() on a unlocked page, triggering VM
layer BUG_ON().
To make subpage compression work at least partially, here we add another
restriction for it, only allow compression if the delalloc range is
fully page aligned.
By that, async extent is always ensured to unlock the first page
exclusively, just like it used to be for regular sectorsize.
In theory, we only need to make sure the delalloc range fully covers its
first page, but the tailing page will be locked anyway, blocking later
writeback until the compression finishes.
Thus here we choose to make sure the range is fully page aligned before
doing the compression.
In the future, we could optimize the situation by properly increase
subpage::writers number for the locked page, but that also means we need
to change how we run delalloc range of page.
(Instead of running each delalloc range we hit, we need to find and lock
all delalloc range covers the page, then run each of them).
Signed-off-by: Qu Wenruo <redacted>
---
fs/btrfs/inode.c | 48 ++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 44 insertions(+), 4 deletions(-)
Moved to misc-next. I did some minor changes in coding style or
changelogs, some of the subject lines were overly long, but otherwise
nothing significant. Most of the changes are pretty heavy and I did not
thoroughly review each expression switching from page to sector, the
additional assertions are great and tests don't complain anymore.
Thanks.