| Message ID | 20201210063905.75727-15-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: add read-only support for subpage sector size | expand |
On 10.12.20 г. 8:39 ч., Qu Wenruo wrote: > To handle subpage status update, add the following new tricks: > - Use btrfs_page_*() helpers to update page status > Now we can handle both cases well. > > - No page unlock for subpage metadata > Since subpage metadata doesn't utilize page locking at all, skip it. > For subpage data locking, it's handled in later commits. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/extent_io.c | 23 +++++++++++++++++------ > 1 file changed, 17 insertions(+), 6 deletions(-) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index 1ec9de2aa910..64a19c1884fc 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -2841,15 +2841,26 @@ static void endio_readpage_release_extent(struct processed_extent *processed, > processed->uptodate = uptodate; > } > > -static void endio_readpage_update_page_status(struct page *page, bool uptodate) > +static void endio_readpage_update_page_status(struct page *page, bool uptodate, > + u64 start, u64 end) > { > + struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb); > + u32 len; > + > + ASSERT(page_offset(page) <= start && > + end <= page_offset(page) + PAGE_SIZE - 1); 'start' in this case is 'start = page_offset(page) + bvec->bv_offset;' from end_bio_extent_readpage, so it can't possibly be less than page_offset, instead it will at least be equal to page_offset if bvec->bv_offset is 0 . However, can we really guarantee this ? You are using the end only for the assert, and given you already have the 'len' parameter calculated in the caller I'd rather have this function take start/len, that would save you from recalculating the len and also for someone looking at the code it would be apparent it's the length of the currently processed bvec. I looked through the end of the series and you never use 'end' just 'len' <snip>
On 2020/12/14 下午5:57, Nikolay Borisov wrote: > > > On 10.12.20 г. 8:39 ч., Qu Wenruo wrote: >> To handle subpage status update, add the following new tricks: >> - Use btrfs_page_*() helpers to update page status >> Now we can handle both cases well. >> >> - No page unlock for subpage metadata >> Since subpage metadata doesn't utilize page locking at all, skip it. >> For subpage data locking, it's handled in later commits. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> fs/btrfs/extent_io.c | 23 +++++++++++++++++------ >> 1 file changed, 17 insertions(+), 6 deletions(-) >> >> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c >> index 1ec9de2aa910..64a19c1884fc 100644 >> --- a/fs/btrfs/extent_io.c >> +++ b/fs/btrfs/extent_io.c >> @@ -2841,15 +2841,26 @@ static void endio_readpage_release_extent(struct processed_extent *processed, >> processed->uptodate = uptodate; >> } >> >> -static void endio_readpage_update_page_status(struct page *page, bool uptodate) >> +static void endio_readpage_update_page_status(struct page *page, bool uptodate, >> + u64 start, u64 end) >> { >> + struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb); >> + u32 len; >> + >> + ASSERT(page_offset(page) <= start && >> + end <= page_offset(page) + PAGE_SIZE - 1); > > 'start' in this case is > 'start = page_offset(page) + bvec->bv_offset;' from > end_bio_extent_readpage, so it can't possibly be less than page_offset, > instead it will at least be equal to page_offset if bvec->bv_offset is 0 > . However, can we really guarantee this ? I believe we can. But as you may have already found, I'm sometimes over-cautious, thus I tend to use ASSERT() as a way to indicate the prerequisites. > > > You are using the end only for the assert, and given you already have > the 'len' parameter calculated in the caller I'd rather have this > function take start/len, that would save you from recalculating the len > and also for someone looking at the code it would be apparent it's the > length of the currently processed bvec. I looked through the end of the > series and you never use 'end' just 'len' Right, sticking len would be better, I'll stick to start/len schema for new functions in the series. Thanks, Qu > > <snip> >
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 1ec9de2aa910..64a19c1884fc 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -2841,15 +2841,26 @@ static void endio_readpage_release_extent(struct processed_extent *processed, processed->uptodate = uptodate; } -static void endio_readpage_update_page_status(struct page *page, bool uptodate) +static void endio_readpage_update_page_status(struct page *page, bool uptodate, + u64 start, u64 end) { + struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb); + u32 len; + + ASSERT(page_offset(page) <= start && + end <= page_offset(page) + PAGE_SIZE - 1); + len = end + 1 - start; + if (uptodate) { - SetPageUptodate(page); + btrfs_page_set_uptodate(fs_info, page, start, len); } else { - ClearPageUptodate(page); - SetPageError(page); + btrfs_page_clear_uptodate(fs_info, page, start, len); + btrfs_page_set_error(fs_info, page, start, len); } - unlock_page(page); + + if (fs_info->sectorsize == PAGE_SIZE) + unlock_page(page); + /* Subpage locking will be handled in later patches */ } /* @@ -2986,7 +2997,7 @@ static void end_bio_extent_readpage(struct bio *bio) bio_offset += len; /* Update page status and unlock */ - endio_readpage_update_page_status(page, uptodate); + endio_readpage_update_page_status(page, uptodate, start, end); endio_readpage_release_extent(&processed, BTRFS_I(inode), start, end, uptodate); }
To handle subpage status update, add the following new tricks: - Use btrfs_page_*() helpers to update page status Now we can handle both cases well. - No page unlock for subpage metadata Since subpage metadata doesn't utilize page locking at all, skip it. For subpage data locking, it's handled in later commits. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/extent_io.c | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)