| Message ID | 20201113125149.140836-11-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: preparation patches for subpage support | expand |
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.
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
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.
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); +} + #define in_range(b, first, len) ((b) >= (first) && (b) < (first) + (len)) /* Sanity test specific functions */
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(+)