| Message ID | 20201021062554.68132-14-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: add basic rw support for subpage sector size | expand |
On Wed, Oct 21, 2020 at 02:24:59PM +0800, Qu Wenruo wrote: > In end_bio_extent_readpage() we had a strange dance around > extent_start/extent_len. > > The truth is, no matter what we're doing using those two variable, the > end result is just the same, clear the EXTENT_LOCKED bit and if needed > set the EXTENT_UPTODATE bit for the io_tree. > > This doesn't need the complex dance, we can do it pretty easily by just > calling endio_readpage_release_extent() for each bvec. > > This greatly streamlines the code. Yes it does, the old code is a series of conditions and new code is just one call but it's hard to see why this is correct. Can you please write some guidance, what are the invariants or how does the logic simplify? What you write above is a summary but for review I'd need something to follow so I don't have to spend too much time reading just this patch. Thanks.
On 2020/10/27 下午6:29, David Sterba wrote: > On Wed, Oct 21, 2020 at 02:24:59PM +0800, Qu Wenruo wrote: >> In end_bio_extent_readpage() we had a strange dance around >> extent_start/extent_len. >> >> The truth is, no matter what we're doing using those two variable, the >> end result is just the same, clear the EXTENT_LOCKED bit and if needed >> set the EXTENT_UPTODATE bit for the io_tree. >> >> This doesn't need the complex dance, we can do it pretty easily by just >> calling endio_readpage_release_extent() for each bvec. >> >> This greatly streamlines the code. > > Yes it does, the old code is a series of conditions and new code is just > one call but it's hard to see why this is correct. Can you please write > some guidance, what are the invariants or how does the logic simplify? > What you write above is a summary but for review I'd need something to > follow so I don't have to spend too much time reading just this patch. > Thanks. > Sorry, I should add more explanation on that, would add that in next update. Thanks, Qu
On Tue, Oct 27, 2020 at 08:15:58PM +0800, Qu Wenruo wrote: > On 2020/10/27 下午6:29, David Sterba wrote: > > On Wed, Oct 21, 2020 at 02:24:59PM +0800, Qu Wenruo wrote: > >> In end_bio_extent_readpage() we had a strange dance around > >> extent_start/extent_len. > >> > >> The truth is, no matter what we're doing using those two variable, the > >> end result is just the same, clear the EXTENT_LOCKED bit and if needed > >> set the EXTENT_UPTODATE bit for the io_tree. > >> > >> This doesn't need the complex dance, we can do it pretty easily by just > >> calling endio_readpage_release_extent() for each bvec. > >> > >> This greatly streamlines the code. > > > > Yes it does, the old code is a series of conditions and new code is just > > one call but it's hard to see why this is correct. Can you please write > > some guidance, what are the invariants or how does the logic simplify? > > What you write above is a summary but for review I'd need something to > > follow so I don't have to spend too much time reading just this patch. > > Thanks. > > > Sorry, I should add more explanation on that, would add that in next update. Most of the patches 1-20 are ok so I've picked them to a branch and will move to misc-next once the comments are answered. I've fixed what I though does not need a resend but for this patch is probably something that you should send and I try to understand. What I have right now is in my github repo in branch ext/qu/subpage-1-prep so you can have a look but you don't need to resend the whole series, if you have updates to any of the patches reply to it and I'll fold it in.
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index ce5b23169e47..3819bf7505e3 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -2791,11 +2791,10 @@ static void end_bio_extent_writepage(struct bio *bio) } static void -endio_readpage_release_extent(struct extent_io_tree *tree, u64 start, u64 len, +endio_readpage_release_extent(struct extent_io_tree *tree, u64 start, u64 end, int uptodate) { struct extent_state *cached = NULL; - u64 end = start + len - 1; if (uptodate && tree->track_uptodate) set_extent_uptodate(tree, start, end, &cached, GFP_ATOMIC); @@ -2823,8 +2822,6 @@ static void end_bio_extent_readpage(struct bio *bio) u64 start; u64 end; u64 len; - u64 extent_start = 0; - u64 extent_len = 0; int mirror; int ret; struct bvec_iter_all iter_all; @@ -2932,32 +2929,9 @@ static void end_bio_extent_readpage(struct bio *bio) unlock_page(page); offset += len; - if (unlikely(!uptodate)) { - if (extent_len) { - endio_readpage_release_extent(tree, - extent_start, - extent_len, 1); - extent_start = 0; - extent_len = 0; - } - endio_readpage_release_extent(tree, start, - end - start + 1, 0); - } else if (!extent_len) { - extent_start = start; - extent_len = end + 1 - start; - } else if (extent_start + extent_len == start) { - extent_len += end + 1 - start; - } else { - endio_readpage_release_extent(tree, extent_start, - extent_len, uptodate); - extent_start = start; - extent_len = end + 1 - start; - } + endio_readpage_release_extent(tree, start, end, uptodate); } - if (extent_len) - endio_readpage_release_extent(tree, extent_start, extent_len, - uptodate); btrfs_io_bio_free_csum(io_bio); bio_put(bio); }
In end_bio_extent_readpage() we had a strange dance around extent_start/extent_len. The truth is, no matter what we're doing using those two variable, the end result is just the same, clear the EXTENT_LOCKED bit and if needed set the EXTENT_UPTODATE bit for the io_tree. This doesn't need the complex dance, we can do it pretty easily by just calling endio_readpage_release_extent() for each bvec. This greatly streamlines the code. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/extent_io.c | 30 ++---------------------------- 1 file changed, 2 insertions(+), 28 deletions(-)