| Message ID | 20210116071533.105780-2-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: add read-only support for subpage sector size | expand |
On 1/16/21 2:15 AM, Qu Wenruo wrote: > When __process_pages_contig() get called for > extent_clear_unlock_delalloc(), if we hit the locked page, only Private2 > bit is updated, but dirty/writeback/error bits are all skipped. > > There are several call sites call extent_clear_unlock_delalloc() with > @locked_page and PAGE_CLEAR_DIRTY/PAGE_SET_WRITEBACK/PAGE_END_WRITEBACK > > - cow_file_range() > - run_delalloc_nocow() > - cow_file_range_async() > All for their error handling branches. > > For those call sites, since we skip the locked page for > dirty/error/writeback bit update, the locked page will still have its > dirty bit remaining. > > Thankfully, since all those call sites can only be hit with various > serious errors, it's pretty hard to hit and shouldn't affect regular > btrfs operations. > > But still, we shouldn't leave the locked_page with its > dirty/error/writeback bits untouched. > > Fix this by only skipping lock/unlock page operations for locked_page. > > Signed-off-by: Qu Wenruo <wqu@suse.com> Except this is handled by the callers. We clear_page_dirty_for_io() the page before calling btrfs_run_delalloc_range(), so we don't need the PAGE_CLEAR_DIRTY, it's already cleared. The SetPageError() is handled in the error path for locked_page, as is the set_writeback/end_writeback. Now I don't think this patch causes problems specifically, but the changelog is at least wrong, and I'd rather we'd skip the handling of the locked_page here and leave it in the proper error handling. If you need to do this for some other reason that I haven't gotten to yet then you need to make that clear in the changelog, because as of right now I don't see why this is needed. Thanks, Josef
On 2021/1/20 上午5:41, Josef Bacik wrote: > On 1/16/21 2:15 AM, Qu Wenruo wrote: >> When __process_pages_contig() get called for >> extent_clear_unlock_delalloc(), if we hit the locked page, only Private2 >> bit is updated, but dirty/writeback/error bits are all skipped. >> >> There are several call sites call extent_clear_unlock_delalloc() with >> @locked_page and PAGE_CLEAR_DIRTY/PAGE_SET_WRITEBACK/PAGE_END_WRITEBACK >> >> - cow_file_range() >> - run_delalloc_nocow() >> - cow_file_range_async() >> All for their error handling branches. >> >> For those call sites, since we skip the locked page for >> dirty/error/writeback bit update, the locked page will still have its >> dirty bit remaining. >> >> Thankfully, since all those call sites can only be hit with various >> serious errors, it's pretty hard to hit and shouldn't affect regular >> btrfs operations. >> >> But still, we shouldn't leave the locked_page with its >> dirty/error/writeback bits untouched. >> >> Fix this by only skipping lock/unlock page operations for locked_page. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> > > Except this is handled by the callers. We clear_page_dirty_for_io() the > page before calling btrfs_run_delalloc_range(), so we don't need the > PAGE_CLEAR_DIRTY, it's already cleared. The SetPageError() is handled > in the error path for locked_page, as is the > set_writeback/end_writeback. Now I don't think this patch causes > problems specifically, but the changelog is at least wrong, and I'd > rather we'd skip the handling of the locked_page here and leave it in > the proper error handling. If you need to do this for some other reason > that I haven't gotten to yet then you need to make that clear in the > changelog, because as of right now I don't see why this is needed. Thanks, This is mostly to co-operate with a later patch on __process_pages_contig(), where we need to make sure page locked by __process_pages_contig() is only unlocked by __process_pages_contig() too. The exception is after cow_file_inline(), we call __process_pages_contig() on the locked page, making it to clear page writeback and unlock it. That is going to cause problems for subpage. Thus I prefer to make __process_pages_contig() to clear page dirty/end writeback for locked page. Thanks, Qu > > Josef
On 2021/1/21 下午2:32, Qu Wenruo wrote: > > > On 2021/1/20 上午5:41, Josef Bacik wrote: >> On 1/16/21 2:15 AM, Qu Wenruo wrote: >>> When __process_pages_contig() get called for >>> extent_clear_unlock_delalloc(), if we hit the locked page, only Private2 >>> bit is updated, but dirty/writeback/error bits are all skipped. >>> >>> There are several call sites call extent_clear_unlock_delalloc() with >>> @locked_page and PAGE_CLEAR_DIRTY/PAGE_SET_WRITEBACK/PAGE_END_WRITEBACK >>> >>> - cow_file_range() >>> - run_delalloc_nocow() >>> - cow_file_range_async() >>> All for their error handling branches. >>> >>> For those call sites, since we skip the locked page for >>> dirty/error/writeback bit update, the locked page will still have its >>> dirty bit remaining. >>> >>> Thankfully, since all those call sites can only be hit with various >>> serious errors, it's pretty hard to hit and shouldn't affect regular >>> btrfs operations. >>> >>> But still, we shouldn't leave the locked_page with its >>> dirty/error/writeback bits untouched. >>> >>> Fix this by only skipping lock/unlock page operations for locked_page. >>> >>> Signed-off-by: Qu Wenruo <wqu@suse.com> >> >> Except this is handled by the callers. We clear_page_dirty_for_io() the >> page before calling btrfs_run_delalloc_range(), so we don't need the >> PAGE_CLEAR_DIRTY, it's already cleared. The SetPageError() is handled >> in the error path for locked_page, as is the >> set_writeback/end_writeback. Now I don't think this patch causes >> problems specifically, but the changelog is at least wrong, and I'd >> rather we'd skip the handling of the locked_page here and leave it in >> the proper error handling. If you need to do this for some other reason >> that I haven't gotten to yet then you need to make that clear in the >> changelog, because as of right now I don't see why this is needed. >> Thanks, > > This is mostly to co-operate with a later patch on > __process_pages_contig(), where we need to make sure page locked by > __process_pages_contig() is only unlocked by __process_pages_contig() too. > > The exception is after cow_file_inline(), we call > __process_pages_contig() on the locked page, making it to clear page > writeback and unlock it. To be more clear, we call extent_clear_unlock_delalloc() with locked_page == NULL, to allow __process_pages_contig() to unlock the locked page (while the locked page isn't locked by __process_pages_contig()). For subpage data, we need writers accounting for subpage, but that accounting only happens in __process_pages_contig(), thus we don't want pages without the accounting to be unlocked by __process_pages_contig(). I can do extra page unlock/clear_dirty/end_writeback just for that exception, but it would definitely needs more comments. Thanks, Qu > > That is going to cause problems for subpage. > > Thus I prefer to make __process_pages_contig() to clear page dirty/end > writeback for locked page. > > Thanks, > Qu >> >> Josef >
On Thu, Jan 21, 2021 at 02:51:46PM +0800, Qu Wenruo wrote: > > > On 2021/1/21 下午2:32, Qu Wenruo wrote: > > > > > > On 2021/1/20 上午5:41, Josef Bacik wrote: > >> On 1/16/21 2:15 AM, Qu Wenruo wrote: > >>> When __process_pages_contig() get called for > >>> extent_clear_unlock_delalloc(), if we hit the locked page, only Private2 > >>> bit is updated, but dirty/writeback/error bits are all skipped. > >>> > >>> There are several call sites call extent_clear_unlock_delalloc() with > >>> @locked_page and PAGE_CLEAR_DIRTY/PAGE_SET_WRITEBACK/PAGE_END_WRITEBACK > >>> > >>> - cow_file_range() > >>> - run_delalloc_nocow() > >>> - cow_file_range_async() > >>> All for their error handling branches. > >>> > >>> For those call sites, since we skip the locked page for > >>> dirty/error/writeback bit update, the locked page will still have its > >>> dirty bit remaining. > >>> > >>> Thankfully, since all those call sites can only be hit with various > >>> serious errors, it's pretty hard to hit and shouldn't affect regular > >>> btrfs operations. > >>> > >>> But still, we shouldn't leave the locked_page with its > >>> dirty/error/writeback bits untouched. > >>> > >>> Fix this by only skipping lock/unlock page operations for locked_page. > >>> > >>> Signed-off-by: Qu Wenruo <wqu@suse.com> > >> > >> Except this is handled by the callers. We clear_page_dirty_for_io() the > >> page before calling btrfs_run_delalloc_range(), so we don't need the > >> PAGE_CLEAR_DIRTY, it's already cleared. The SetPageError() is handled > >> in the error path for locked_page, as is the > >> set_writeback/end_writeback. Now I don't think this patch causes > >> problems specifically, but the changelog is at least wrong, and I'd > >> rather we'd skip the handling of the locked_page here and leave it in > >> the proper error handling. If you need to do this for some other reason > >> that I haven't gotten to yet then you need to make that clear in the > >> changelog, because as of right now I don't see why this is needed. > >> Thanks, > > > > This is mostly to co-operate with a later patch on > > __process_pages_contig(), where we need to make sure page locked by > > __process_pages_contig() is only unlocked by __process_pages_contig() too. > > > > The exception is after cow_file_inline(), we call > > __process_pages_contig() on the locked page, making it to clear page > > writeback and unlock it. > > To be more clear, we call extent_clear_unlock_delalloc() with > locked_page == NULL, to allow __process_pages_contig() to unlock the > locked page (while the locked page isn't locked by > __process_pages_contig()). > > For subpage data, we need writers accounting for subpage, but that > accounting only happens in __process_pages_contig(), thus we don't want > pages without the accounting to be unlocked by __process_pages_contig(). > > I can do extra page unlock/clear_dirty/end_writeback just for that > exception, but it would definitely needs more comments. This is patch 1 and other depend on the changed behaviour so if it's just updated changelog and comments, and Josef is ok with the result, I can take it but otherwise this could delay the series once the rest is reworked.
On 2021/1/24 上午3:13, David Sterba wrote: > On Thu, Jan 21, 2021 at 02:51:46PM +0800, Qu Wenruo wrote: >> >> >> On 2021/1/21 下午2:32, Qu Wenruo wrote: >>> >>> >>> On 2021/1/20 上午5:41, Josef Bacik wrote: >>>> On 1/16/21 2:15 AM, Qu Wenruo wrote: >>>>> When __process_pages_contig() get called for >>>>> extent_clear_unlock_delalloc(), if we hit the locked page, only Private2 >>>>> bit is updated, but dirty/writeback/error bits are all skipped. >>>>> >>>>> There are several call sites call extent_clear_unlock_delalloc() with >>>>> @locked_page and PAGE_CLEAR_DIRTY/PAGE_SET_WRITEBACK/PAGE_END_WRITEBACK >>>>> >>>>> - cow_file_range() >>>>> - run_delalloc_nocow() >>>>> - cow_file_range_async() >>>>> All for their error handling branches. >>>>> >>>>> For those call sites, since we skip the locked page for >>>>> dirty/error/writeback bit update, the locked page will still have its >>>>> dirty bit remaining. >>>>> >>>>> Thankfully, since all those call sites can only be hit with various >>>>> serious errors, it's pretty hard to hit and shouldn't affect regular >>>>> btrfs operations. >>>>> >>>>> But still, we shouldn't leave the locked_page with its >>>>> dirty/error/writeback bits untouched. >>>>> >>>>> Fix this by only skipping lock/unlock page operations for locked_page. >>>>> >>>>> Signed-off-by: Qu Wenruo <wqu@suse.com> >>>> >>>> Except this is handled by the callers. We clear_page_dirty_for_io() the >>>> page before calling btrfs_run_delalloc_range(), so we don't need the >>>> PAGE_CLEAR_DIRTY, it's already cleared. The SetPageError() is handled >>>> in the error path for locked_page, as is the >>>> set_writeback/end_writeback. Now I don't think this patch causes >>>> problems specifically, but the changelog is at least wrong, and I'd >>>> rather we'd skip the handling of the locked_page here and leave it in >>>> the proper error handling. If you need to do this for some other reason >>>> that I haven't gotten to yet then you need to make that clear in the >>>> changelog, because as of right now I don't see why this is needed. >>>> Thanks, >>> >>> This is mostly to co-operate with a later patch on >>> __process_pages_contig(), where we need to make sure page locked by >>> __process_pages_contig() is only unlocked by __process_pages_contig() too. >>> >>> The exception is after cow_file_inline(), we call >>> __process_pages_contig() on the locked page, making it to clear page >>> writeback and unlock it. >> >> To be more clear, we call extent_clear_unlock_delalloc() with >> locked_page == NULL, to allow __process_pages_contig() to unlock the >> locked page (while the locked page isn't locked by >> __process_pages_contig()). >> >> For subpage data, we need writers accounting for subpage, but that >> accounting only happens in __process_pages_contig(), thus we don't want >> pages without the accounting to be unlocked by __process_pages_contig(). >> >> I can do extra page unlock/clear_dirty/end_writeback just for that >> exception, but it would definitely needs more comments. > > This is patch 1 and other depend on the changed behaviour so if it's > just updated changelog and comments, and Josef is ok with the result, I > can take it but otherwise this could delay the series once the rest is > reworked. > In fact there aren't many changes depending on it, until we hit RW support. Thus I can move this patch to RW series, so that we can fully focus on RO support. The patchset will be delayed for a while (ETA in week 04), mostly due to the change in how we handle metadata ref count (other than just under_alloc). Would this be OK for you? Thanks, Qu
On Sun, Jan 24, 2021 at 08:35:27AM +0800, Qu Wenruo wrote: > On 2021/1/24 上午3:13, David Sterba wrote: > > On Thu, Jan 21, 2021 at 02:51:46PM +0800, Qu Wenruo wrote: > >> On 2021/1/21 下午2:32, Qu Wenruo wrote: > >>> On 2021/1/20 上午5:41, Josef Bacik wrote: > >>>> On 1/16/21 2:15 AM, Qu Wenruo wrote: > >> To be more clear, we call extent_clear_unlock_delalloc() with > >> locked_page == NULL, to allow __process_pages_contig() to unlock the > >> locked page (while the locked page isn't locked by > >> __process_pages_contig()). > >> > >> For subpage data, we need writers accounting for subpage, but that > >> accounting only happens in __process_pages_contig(), thus we don't want > >> pages without the accounting to be unlocked by __process_pages_contig(). > >> > >> I can do extra page unlock/clear_dirty/end_writeback just for that > >> exception, but it would definitely needs more comments. > > > > This is patch 1 and other depend on the changed behaviour so if it's > > just updated changelog and comments, and Josef is ok with the result, I > > can take it but otherwise this could delay the series once the rest is > > reworked. > > > In fact there aren't many changes depending on it, until we hit RW support. > > Thus I can move this patch to RW series, so that we can fully focus on > RO support. That's a good option. > The patchset will be delayed for a while (ETA in week 04), mostly due to > the change in how we handle metadata ref count (other than just > under_alloc). > > Would this be OK for you? Yes that OK, thanks.
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 7f689ad7709c..3442f1746683 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -1970,11 +1970,6 @@ static int __process_pages_contig(struct address_space *mapping, if (page_ops & PAGE_SET_PRIVATE2) SetPagePrivate2(pages[i]); - if (locked_page && pages[i] == locked_page) { - put_page(pages[i]); - pages_processed++; - continue; - } if (page_ops & PAGE_CLEAR_DIRTY) clear_page_dirty_for_io(pages[i]); if (page_ops & PAGE_SET_WRITEBACK) @@ -1983,6 +1978,11 @@ static int __process_pages_contig(struct address_space *mapping, SetPageError(pages[i]); if (page_ops & PAGE_END_WRITEBACK) end_page_writeback(pages[i]); + if (locked_page && pages[i] == locked_page) { + put_page(pages[i]); + pages_processed++; + continue; + } if (page_ops & PAGE_UNLOCK) unlock_page(pages[i]); if (page_ops & PAGE_LOCK) {
When __process_pages_contig() get called for extent_clear_unlock_delalloc(), if we hit the locked page, only Private2 bit is updated, but dirty/writeback/error bits are all skipped. There are several call sites call extent_clear_unlock_delalloc() with @locked_page and PAGE_CLEAR_DIRTY/PAGE_SET_WRITEBACK/PAGE_END_WRITEBACK - cow_file_range() - run_delalloc_nocow() - cow_file_range_async() All for their error handling branches. For those call sites, since we skip the locked page for dirty/error/writeback bit update, the locked page will still have its dirty bit remaining. Thankfully, since all those call sites can only be hit with various serious errors, it's pretty hard to hit and shouldn't affect regular btrfs operations. But still, we shouldn't leave the locked_page with its dirty/error/writeback bits untouched. Fix this by only skipping lock/unlock page operations for locked_page. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/extent_io.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)