| Message ID | 20201103133108.148112-5-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/32] btrfs: extent_io: remove the extent_start/extent_len for end_bio_extent_readpage() | expand |
On 3.11.20 г. 15:30 ч., Qu Wenruo wrote: > In btree_write_cache_pages() we have a btree page submission routine > buried deeply into a nested loop. > > This patch will extract that part of code into a helper function, > submit_btree_page(), to do the same work. > > Also, since submit_btree_page() now can return >0 for successfull extent > buffer submission, remove the "ASSERT(ret <= 0);" line. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > Signed-off-by: David Sterba <dsterba@suse.com> > --- > fs/btrfs/extent_io.c | 116 +++++++++++++++++++++++++------------------ > 1 file changed, 69 insertions(+), 47 deletions(-) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index 9cbce0b74db7..ac396d8937b9 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -3935,10 +3935,75 @@ static noinline_for_stack int write_one_eb(struct extent_buffer *eb, > return ret; > } > > +/* > + * A helper to submit a btree page. > + * > + * This function is not always submitting the page, as we only submit the full > + * extent buffer in a batch. > + * > + * @page: The btree page > + * @prev_eb: Previous extent buffer, to determine if we need to submit > + * this page. > + * nit: Add all parameters. > + * Return >0 if we have submitted the extent buffer successfully. > + * Return 0 if we don't need to do anything for the page. > + * Return <0 for fatal error. > + */ > +static int submit_btree_page(struct page *page, struct writeback_control *wbc, > + struct extent_page_data *epd, > + struct extent_buffer **prev_eb) Rename prev_eb to eb_context/eb_ctx or simply eb. That's not really "previous", we essentially want to skip all but first page of an eb since we anyway iterate all pages in write_one_eb. I guess this could be described as part of the argument's documentation. <snip>
On Thu, Nov 05, 2020 at 12:47:32PM +0200, Nikolay Borisov wrote: > > > On 3.11.20 г. 15:30 ч., Qu Wenruo wrote: > > In btree_write_cache_pages() we have a btree page submission routine > > buried deeply into a nested loop. > > > > This patch will extract that part of code into a helper function, > > submit_btree_page(), to do the same work. > > > > Also, since submit_btree_page() now can return >0 for successfull extent > > buffer submission, remove the "ASSERT(ret <= 0);" line. > > > > Signed-off-by: Qu Wenruo <wqu@suse.com> > > Signed-off-by: David Sterba <dsterba@suse.com> > > --- > > fs/btrfs/extent_io.c | 116 +++++++++++++++++++++++++------------------ > > 1 file changed, 69 insertions(+), 47 deletions(-) > > > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > > index 9cbce0b74db7..ac396d8937b9 100644 > > --- a/fs/btrfs/extent_io.c > > +++ b/fs/btrfs/extent_io.c > > @@ -3935,10 +3935,75 @@ static noinline_for_stack int write_one_eb(struct extent_buffer *eb, > > return ret; > > } > > > > +/* > > + * A helper to submit a btree page. > > + * > > + * This function is not always submitting the page, as we only submit the full > > + * extent buffer in a batch. This is confusing, it's submitting conditionally eb pages, so the main object is eb not the pages, so it should be probably submit_eb_page.
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 9cbce0b74db7..ac396d8937b9 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -3935,10 +3935,75 @@ static noinline_for_stack int write_one_eb(struct extent_buffer *eb, return ret; } +/* + * A helper to submit a btree page. + * + * This function is not always submitting the page, as we only submit the full + * extent buffer in a batch. + * + * @page: The btree page + * @prev_eb: Previous extent buffer, to determine if we need to submit + * this page. + * + * Return >0 if we have submitted the extent buffer successfully. + * Return 0 if we don't need to do anything for the page. + * Return <0 for fatal error. + */ +static int submit_btree_page(struct page *page, struct writeback_control *wbc, + struct extent_page_data *epd, + struct extent_buffer **prev_eb) +{ + struct address_space *mapping = page->mapping; + struct extent_buffer *eb; + int ret; + + if (!PagePrivate(page)) + return 0; + + spin_lock(&mapping->private_lock); + if (!PagePrivate(page)) { + spin_unlock(&mapping->private_lock); + return 0; + } + + eb = (struct extent_buffer *)page->private; + + /* + * Shouldn't happen and normally this would be a BUG_ON but no sense + * in crashing the users box for something we can survive anyway. + */ + if (WARN_ON(!eb)) { + spin_unlock(&mapping->private_lock); + return 0; + } + + if (eb == *prev_eb) { + spin_unlock(&mapping->private_lock); + return 0; + } + ret = atomic_inc_not_zero(&eb->refs); + spin_unlock(&mapping->private_lock); + if (!ret) + return 0; + + *prev_eb = eb; + + ret = lock_extent_buffer_for_io(eb, epd); + if (ret <= 0) { + free_extent_buffer(eb); + return ret; + } + ret = write_one_eb(eb, wbc, epd); + free_extent_buffer(eb); + if (ret < 0) + return ret; + return 1; +} + int btree_write_cache_pages(struct address_space *mapping, struct writeback_control *wbc) { - struct extent_buffer *eb, *prev_eb = NULL; + struct extent_buffer *prev_eb = NULL; struct extent_page_data epd = { .bio = NULL, .extent_locked = 0, @@ -3984,55 +4049,13 @@ int btree_write_cache_pages(struct address_space *mapping, for (i = 0; i < nr_pages; i++) { struct page *page = pvec.pages[i]; - if (!PagePrivate(page)) - continue; - - spin_lock(&mapping->private_lock); - if (!PagePrivate(page)) { - spin_unlock(&mapping->private_lock); - continue; - } - - eb = (struct extent_buffer *)page->private; - - /* - * Shouldn't happen and normally this would be a BUG_ON - * but no sense in crashing the users box for something - * we can survive anyway. - */ - if (WARN_ON(!eb)) { - spin_unlock(&mapping->private_lock); - continue; - } - - if (eb == prev_eb) { - spin_unlock(&mapping->private_lock); - continue; - } - - ret = atomic_inc_not_zero(&eb->refs); - spin_unlock(&mapping->private_lock); - if (!ret) - continue; - - prev_eb = eb; - ret = lock_extent_buffer_for_io(eb, &epd); - if (!ret) { - free_extent_buffer(eb); + ret = submit_btree_page(page, wbc, &epd, &prev_eb); + if (ret == 0) continue; - } else if (ret < 0) { - done = 1; - free_extent_buffer(eb); - break; - } - - ret = write_one_eb(eb, wbc, &epd); - if (ret) { + if (ret < 0) { done = 1; - free_extent_buffer(eb); break; } - free_extent_buffer(eb); /* * the filesystem may choose to bump up nr_to_write. @@ -4053,7 +4076,6 @@ int btree_write_cache_pages(struct address_space *mapping, index = 0; goto retry; } - ASSERT(ret <= 0); if (ret < 0) { end_write_bio(&epd, ret); return ret;