| Message ID | 20190218052753.24138-11-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: Enhancement to tree block validation | expand |
On Mon, Feb 18, 2019 at 01:27:51PM +0800, Qu Wenruo wrote: > Signed-off-by: Qu Wenruo <wqu@suse.com> > Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de> > --- > fs/btrfs/extent_io.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index 1572e892ec7b..480e138051f0 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -3998,7 +3998,10 @@ static int extent_write_cache_pages(struct address_space *mapping, > */ > if (!trylock_page(page)) { > ret = flush_write_bio(epd); > - BUG_ON(ret < 0); > + if (ret < 0) { This needs some more explanation why it's correct, there's conditional locking and writeback status manipulation. Thanks.
On 2019/3/12 上午8:33, David Sterba wrote: > On Mon, Feb 18, 2019 at 01:27:51PM +0800, Qu Wenruo wrote: >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de> >> --- >> fs/btrfs/extent_io.c | 10 ++++++++-- >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c >> index 1572e892ec7b..480e138051f0 100644 >> --- a/fs/btrfs/extent_io.c >> +++ b/fs/btrfs/extent_io.c >> @@ -3998,7 +3998,10 @@ static int extent_write_cache_pages(struct address_space *mapping, >> */ >> if (!trylock_page(page)) { >> ret = flush_write_bio(epd); >> - BUG_ON(ret < 0); >> + if (ret < 0) { > > This needs some more explanation why it's correct, there's conditional > locking and writeback status manipulation. Thanks. Because flush_write_bio() handles the cleanup in its endio function. Thus we don't need extra cleanup in this case. Thanks, Qu
On Tue, Mar 12, 2019 at 08:42:12AM +0800, Qu Wenruo wrote: > > > On 2019/3/12 上午8:33, David Sterba wrote: > > On Mon, Feb 18, 2019 at 01:27:51PM +0800, Qu Wenruo wrote: > >> Signed-off-by: Qu Wenruo <wqu@suse.com> > >> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de> > >> --- > >> fs/btrfs/extent_io.c | 10 ++++++++-- > >> 1 file changed, 8 insertions(+), 2 deletions(-) > >> > >> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > >> index 1572e892ec7b..480e138051f0 100644 > >> --- a/fs/btrfs/extent_io.c > >> +++ b/fs/btrfs/extent_io.c > >> @@ -3998,7 +3998,10 @@ static int extent_write_cache_pages(struct address_space *mapping, > >> */ > >> if (!trylock_page(page)) { > >> ret = flush_write_bio(epd); > >> - BUG_ON(ret < 0); > >> + if (ret < 0) { > > > > This needs some more explanation why it's correct, there's conditional > > locking and writeback status manipulation. Thanks. > > Because flush_write_bio() handles the cleanup in its endio function. > > Thus we don't need extra cleanup in this case. Non-trivial changes need changelog, that's nothing new. Especially for error handling you have to provide a proof that things don't get broken. If you spend a week working on the code, everything is trivial, but only for you and for a short period of time. That's why we need good changelogs that make sense to other people and after a long period of time. Thanks.
On 2019/3/13 下午7:31, David Sterba wrote: > On Tue, Mar 12, 2019 at 08:42:12AM +0800, Qu Wenruo wrote: >> >> >> On 2019/3/12 上午8:33, David Sterba wrote: >>> On Mon, Feb 18, 2019 at 01:27:51PM +0800, Qu Wenruo wrote: >>>> Signed-off-by: Qu Wenruo <wqu@suse.com> >>>> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de> >>>> --- >>>> fs/btrfs/extent_io.c | 10 ++++++++-- >>>> 1 file changed, 8 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c >>>> index 1572e892ec7b..480e138051f0 100644 >>>> --- a/fs/btrfs/extent_io.c >>>> +++ b/fs/btrfs/extent_io.c >>>> @@ -3998,7 +3998,10 @@ static int extent_write_cache_pages(struct address_space *mapping, >>>> */ >>>> if (!trylock_page(page)) { >>>> ret = flush_write_bio(epd); >>>> - BUG_ON(ret < 0); >>>> + if (ret < 0) { >>> >>> This needs some more explanation why it's correct, there's conditional >>> locking and writeback status manipulation. Thanks. >> >> Because flush_write_bio() handles the cleanup in its endio function. >> >> Thus we don't need extra cleanup in this case. > > Non-trivial changes need changelog, that's nothing new. Especially for > error handling you have to provide a proof that things don't get broken. > > If you spend a week working on the code, everything is trivial, but only > for you and for a short period of time. That's why we need good > changelogs that make sense to other people and after a long period of > time. Thanks. That completely makes sense, since even myself need some time to get to the point why the error out works. I'll update the change log for a comprehensive call path why it works. Thanks, Qu
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 1572e892ec7b..480e138051f0 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -3998,7 +3998,10 @@ static int extent_write_cache_pages(struct address_space *mapping, */ if (!trylock_page(page)) { ret = flush_write_bio(epd); - BUG_ON(ret < 0); + if (ret < 0) { + done = 1; + break; + } lock_page(page); } @@ -4010,7 +4013,10 @@ static int extent_write_cache_pages(struct address_space *mapping, if (wbc->sync_mode != WB_SYNC_NONE) { if (PageWriteback(page)) { ret = flush_write_bio(epd); - BUG_ON(ret < 0); + if (ret < 0) { + done = 1; + break; + } } wait_on_page_writeback(page); }