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?


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?

Thanks,
Qu