Re: [PATCH v2 10/24] btrfs: introduce a helper to determine if the sectorsize is smaller than PAGE_SIZE

[David Sterba <dsterba@suse.cz>]

On Tue, Nov 17, 2020 at 07:50:48AM +0800, Qu Wenruo wrote:
> On 2020/11/17 上午6:51, David Sterba wrote:
> > On Fri, Nov 13, 2020 at 08:51:35PM +0800, Qu Wenruo wrote:
> >> Just to save us several letters for the incoming patches.
> >>
> >> Signed-off-by: Qu Wenruo <wqu@suse.com>
> >> ---
> >>  fs/btrfs/ctree.h | 5 +++++
> >>  1 file changed, 5 insertions(+)
> >>
> >> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> >> index 99955b6bfc62..93de6134c65c 100644
> >> --- a/fs/btrfs/ctree.h
> >> +++ b/fs/btrfs/ctree.h
> >> @@ -3660,6 +3660,11 @@ static inline int btrfs_defrag_cancelled(struct btrfs_fs_info *fs_info)
> >>  	return signal_pending(current);
> >>  }
> >>  
> >> +static inline bool btrfs_is_subpage(struct btrfs_fs_info *fs_info)
> >> +{
> >> +	return (fs_info->sectorsize < PAGE_SIZE);
> > 
> > I'm not convinced we want to obscure the simple check, saving a few
> > letters does not sound like a compelling argument. Nothing wrong to have
> > 'sectorsize < PAGE_SIZE' in conditions.
> > 
> OK, I can go without the helper.
> 
> But there are more things like:
> 
> struct btrfs_fs_info *fs_info = btrfs_sb(page->mapping->host->i_sb);
> 
> Should we use a helper or just above line?

That's ok and we can clean it up later, the pointer chain is used in
several places so that's not an uncommon pattern and if we want to use
something compact then it's better to do that all at once.

> Since we're here, allow me to ask for some advice on how to refactor
> some code.
> 
> Another question is in my current RW branch.
> We will have quite some calls like:
> 
> spin_lock(&subpage->lock);
> bitmap_set(subpage->uptodate_bitmap, bit_start, nbits);
> if (bitmap_full(subpage->uptodate_bitmap, BTRFS_SUBPAGE_BITMAP_SIZE))
> 	SetPageUptodate(page);
> spin_unlock(&subpage->lock);
> 
> The call sites are not that many, <=5, but there are several different
> combinations, e.g. for endio we want to use irqsave version of spinlock.
> 
> For data write, we want to convert bits in dirty_bitmap to
> writeback_bitmap, and re-check page status.
> 
> Should I introduce some helpers for that too?

I haven't seen the code but from what you say I think it could be hard
to have one common helper for all the cases. I can give you another
oppinion after I see the actual patch so if the code follows a pattern
lock/update bitmap/check/nulock I think it's ok.