Message ID | 147873187285.2820.9992440620792982440.stgit@birch.djwong.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 11/10/2016 06:51 AM, Darrick J. Wong wrote: > Replace the open-coded inode refcount flag test with a helper function > to reduce the potential for bugs. Thanks for this series;-) Some comments inline below: > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > fs/ocfs2/refcounttree.c | 28 +++++++++++++++------------- > fs/ocfs2/refcounttree.h | 2 ++ > 2 files changed, 17 insertions(+), 13 deletions(-) > > > diff --git a/fs/ocfs2/refcounttree.c b/fs/ocfs2/refcounttree.c > index 1923851..59be8f4 100644 > --- a/fs/ocfs2/refcounttree.c > +++ b/fs/ocfs2/refcounttree.c > @@ -48,6 +48,12 @@ > #include <linux/mount.h> > #include <linux/posix_acl.h> > > +/* Does this inode have the reflink flag set? */ > +bool ocfs2_is_refcount_inode(struct inode *inode) Should it be an inline function? After applying this patch, looks there are still some places not being replaced with this function: --- fs/ocfs2 # grep -rn "OCFS2_I(inode)->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL" xattr.c:2580: if (OCFS2_I(inode)->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL) { xattr.c:3611: if (OCFS2_I(inode)->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL && file.c:1722: if (OCFS2_I(inode)->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL) { file.c:2039: !(OCFS2_I(inode)->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL) || refcounttree.c:55: return (OCFS2_I(inode)->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL); Eric > +{ > + return (OCFS2_I(inode)->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL); > +} > + > struct ocfs2_cow_context { > struct inode *inode; > u32 cow_start; > @@ -410,7 +416,7 @@ static int ocfs2_get_refcount_block(struct inode *inode, u64 *ref_blkno) > goto out; > } > > - BUG_ON(!(OCFS2_I(inode)->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL)); > + BUG_ON(!ocfs2_is_refcount_inode(inode)); > > di = (struct ocfs2_dinode *)di_bh->b_data; > *ref_blkno = le64_to_cpu(di->i_refcount_loc); > @@ -570,7 +576,7 @@ static int ocfs2_create_refcount_tree(struct inode *inode, > u32 num_got; > u64 suballoc_loc, first_blkno; > > - BUG_ON(oi->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL); > + BUG_ON(ocfs2_is_refcount_inode(inode)); > > trace_ocfs2_create_refcount_tree( > (unsigned long long)OCFS2_I(inode)->ip_blkno); > @@ -708,7 +714,7 @@ static int ocfs2_set_refcount_tree(struct inode *inode, > struct ocfs2_refcount_block *rb; > struct ocfs2_refcount_tree *ref_tree; > > - BUG_ON(oi->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL); > + BUG_ON(ocfs2_is_refcount_inode(inode)); > > ret = ocfs2_lock_refcount_tree(osb, refcount_loc, 1, > &ref_tree, &ref_root_bh); > @@ -775,7 +781,7 @@ int ocfs2_remove_refcount_tree(struct inode *inode, struct buffer_head *di_bh) > u64 blk = 0, bg_blkno = 0, ref_blkno = le64_to_cpu(di->i_refcount_loc); > u16 bit = 0; > > - if (!(oi->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL)) > + if (!ocfs2_is_refcount_inode(inode)) > return 0; > > BUG_ON(!ref_blkno); > @@ -2299,11 +2305,10 @@ int ocfs2_decrease_refcount(struct inode *inode, > { > int ret; > u64 ref_blkno; > - struct ocfs2_inode_info *oi = OCFS2_I(inode); > struct buffer_head *ref_root_bh = NULL; > struct ocfs2_refcount_tree *tree; > > - BUG_ON(!(oi->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL)); > + BUG_ON(!ocfs2_is_refcount_inode(inode)); > > ret = ocfs2_get_refcount_block(inode, &ref_blkno); > if (ret) { > @@ -2533,7 +2538,6 @@ int ocfs2_prepare_refcount_change_for_del(struct inode *inode, > int *ref_blocks) > { > int ret; > - struct ocfs2_inode_info *oi = OCFS2_I(inode); > struct buffer_head *ref_root_bh = NULL; > struct ocfs2_refcount_tree *tree; > u64 start_cpos = ocfs2_blocks_to_clusters(inode->i_sb, phys_blkno); > @@ -2544,7 +2548,7 @@ int ocfs2_prepare_refcount_change_for_del(struct inode *inode, > goto out; > } > > - BUG_ON(!(oi->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL)); > + BUG_ON(!ocfs2_is_refcount_inode(inode)); > > ret = ocfs2_get_refcount_tree(OCFS2_SB(inode->i_sb), > refcount_loc, &tree); > @@ -3412,14 +3416,13 @@ static int ocfs2_refcount_cow_hunk(struct inode *inode, > { > int ret; > u32 cow_start = 0, cow_len = 0; > - struct ocfs2_inode_info *oi = OCFS2_I(inode); > struct ocfs2_super *osb = OCFS2_SB(inode->i_sb); > struct ocfs2_dinode *di = (struct ocfs2_dinode *)di_bh->b_data; > struct buffer_head *ref_root_bh = NULL; > struct ocfs2_refcount_tree *ref_tree; > struct ocfs2_cow_context *context = NULL; > > - BUG_ON(!(oi->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL)); > + BUG_ON(!ocfs2_is_refcount_inode(inode)); > > ret = ocfs2_refcount_cal_cow_clusters(inode, &di->id2.i_list, > cpos, write_len, max_cpos, > @@ -3629,11 +3632,10 @@ int ocfs2_refcount_cow_xattr(struct inode *inode, > { > int ret; > struct ocfs2_xattr_value_root *xv = vb->vb_xv; > - struct ocfs2_inode_info *oi = OCFS2_I(inode); > struct ocfs2_cow_context *context = NULL; > u32 cow_start, cow_len; > > - BUG_ON(!(oi->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL)); > + BUG_ON(!ocfs2_is_refcount_inode(inode)); > > ret = ocfs2_refcount_cal_cow_clusters(inode, &xv->xr_list, > cpos, write_len, UINT_MAX, > @@ -3807,7 +3809,7 @@ static int ocfs2_attach_refcount_tree(struct inode *inode, > > ocfs2_init_dealloc_ctxt(&dealloc); > > - if (!(oi->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL)) { > + if (!ocfs2_is_refcount_inode(inode)) { > ret = ocfs2_create_refcount_tree(inode, di_bh); > if (ret) { > mlog_errno(ret); > diff --git a/fs/ocfs2/refcounttree.h b/fs/ocfs2/refcounttree.h > index 6422bbc..553edfb 100644 > --- a/fs/ocfs2/refcounttree.h > +++ b/fs/ocfs2/refcounttree.h > @@ -17,6 +17,8 @@ > #ifndef OCFS2_REFCOUNTTREE_H > #define OCFS2_REFCOUNTTREE_H > > +bool ocfs2_is_refcount_inode(struct inode *inode); > + > struct ocfs2_refcount_tree { > struct rb_node rf_node; > u64 rf_blkno; > > -- > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > >
On Thu, Nov 10, 2016 at 10:14:48AM +0800, Eric Ren wrote: > On 11/10/2016 06:51 AM, Darrick J. Wong wrote: > >Replace the open-coded inode refcount flag test with a helper function > >to reduce the potential for bugs. > Thanks for this series;-) Some comments inline below: > > > >Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> > >--- > > fs/ocfs2/refcounttree.c | 28 +++++++++++++++------------- > > fs/ocfs2/refcounttree.h | 2 ++ > > 2 files changed, 17 insertions(+), 13 deletions(-) > > > > > >diff --git a/fs/ocfs2/refcounttree.c b/fs/ocfs2/refcounttree.c > >index 1923851..59be8f4 100644 > >--- a/fs/ocfs2/refcounttree.c > >+++ b/fs/ocfs2/refcounttree.c > >@@ -48,6 +48,12 @@ > > #include <linux/mount.h> > > #include <linux/posix_acl.h> > >+/* Does this inode have the reflink flag set? */ > >+bool ocfs2_is_refcount_inode(struct inode *inode) > Should it be an inline function? Yes, it can be an inline function. > After applying this patch, looks there are still some places not being > replaced with this function: > --- > fs/ocfs2 # grep -rn "OCFS2_I(inode)->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL" > xattr.c:2580: if (OCFS2_I(inode)->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL) { > xattr.c:3611: if (OCFS2_I(inode)->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL && > file.c:1722: if (OCFS2_I(inode)->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL) { > file.c:2039: !(OCFS2_I(inode)->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL) || > refcounttree.c:55: return (OCFS2_I(inode)->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL); Oops. Yeah, I missed those. Will send out a v2 patch. --D > > Eric > > >+{ > >+ return (OCFS2_I(inode)->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL); > >+} > >+ > > struct ocfs2_cow_context { > > struct inode *inode; > > u32 cow_start; > >@@ -410,7 +416,7 @@ static int ocfs2_get_refcount_block(struct inode *inode, u64 *ref_blkno) > > goto out; > > } > >- BUG_ON(!(OCFS2_I(inode)->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL)); > >+ BUG_ON(!ocfs2_is_refcount_inode(inode)); > > di = (struct ocfs2_dinode *)di_bh->b_data; > > *ref_blkno = le64_to_cpu(di->i_refcount_loc); > >@@ -570,7 +576,7 @@ static int ocfs2_create_refcount_tree(struct inode *inode, > > u32 num_got; > > u64 suballoc_loc, first_blkno; > >- BUG_ON(oi->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL); > >+ BUG_ON(ocfs2_is_refcount_inode(inode)); > > trace_ocfs2_create_refcount_tree( > > (unsigned long long)OCFS2_I(inode)->ip_blkno); > >@@ -708,7 +714,7 @@ static int ocfs2_set_refcount_tree(struct inode *inode, > > struct ocfs2_refcount_block *rb; > > struct ocfs2_refcount_tree *ref_tree; > >- BUG_ON(oi->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL); > >+ BUG_ON(ocfs2_is_refcount_inode(inode)); > > ret = ocfs2_lock_refcount_tree(osb, refcount_loc, 1, > > &ref_tree, &ref_root_bh); > >@@ -775,7 +781,7 @@ int ocfs2_remove_refcount_tree(struct inode *inode, struct buffer_head *di_bh) > > u64 blk = 0, bg_blkno = 0, ref_blkno = le64_to_cpu(di->i_refcount_loc); > > u16 bit = 0; > >- if (!(oi->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL)) > >+ if (!ocfs2_is_refcount_inode(inode)) > > return 0; > > BUG_ON(!ref_blkno); > >@@ -2299,11 +2305,10 @@ int ocfs2_decrease_refcount(struct inode *inode, > > { > > int ret; > > u64 ref_blkno; > >- struct ocfs2_inode_info *oi = OCFS2_I(inode); > > struct buffer_head *ref_root_bh = NULL; > > struct ocfs2_refcount_tree *tree; > >- BUG_ON(!(oi->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL)); > >+ BUG_ON(!ocfs2_is_refcount_inode(inode)); > > ret = ocfs2_get_refcount_block(inode, &ref_blkno); > > if (ret) { > >@@ -2533,7 +2538,6 @@ int ocfs2_prepare_refcount_change_for_del(struct inode *inode, > > int *ref_blocks) > > { > > int ret; > >- struct ocfs2_inode_info *oi = OCFS2_I(inode); > > struct buffer_head *ref_root_bh = NULL; > > struct ocfs2_refcount_tree *tree; > > u64 start_cpos = ocfs2_blocks_to_clusters(inode->i_sb, phys_blkno); > >@@ -2544,7 +2548,7 @@ int ocfs2_prepare_refcount_change_for_del(struct inode *inode, > > goto out; > > } > >- BUG_ON(!(oi->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL)); > >+ BUG_ON(!ocfs2_is_refcount_inode(inode)); > > ret = ocfs2_get_refcount_tree(OCFS2_SB(inode->i_sb), > > refcount_loc, &tree); > >@@ -3412,14 +3416,13 @@ static int ocfs2_refcount_cow_hunk(struct inode *inode, > > { > > int ret; > > u32 cow_start = 0, cow_len = 0; > >- struct ocfs2_inode_info *oi = OCFS2_I(inode); > > struct ocfs2_super *osb = OCFS2_SB(inode->i_sb); > > struct ocfs2_dinode *di = (struct ocfs2_dinode *)di_bh->b_data; > > struct buffer_head *ref_root_bh = NULL; > > struct ocfs2_refcount_tree *ref_tree; > > struct ocfs2_cow_context *context = NULL; > >- BUG_ON(!(oi->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL)); > >+ BUG_ON(!ocfs2_is_refcount_inode(inode)); > > ret = ocfs2_refcount_cal_cow_clusters(inode, &di->id2.i_list, > > cpos, write_len, max_cpos, > >@@ -3629,11 +3632,10 @@ int ocfs2_refcount_cow_xattr(struct inode *inode, > > { > > int ret; > > struct ocfs2_xattr_value_root *xv = vb->vb_xv; > >- struct ocfs2_inode_info *oi = OCFS2_I(inode); > > struct ocfs2_cow_context *context = NULL; > > u32 cow_start, cow_len; > >- BUG_ON(!(oi->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL)); > >+ BUG_ON(!ocfs2_is_refcount_inode(inode)); > > ret = ocfs2_refcount_cal_cow_clusters(inode, &xv->xr_list, > > cpos, write_len, UINT_MAX, > >@@ -3807,7 +3809,7 @@ static int ocfs2_attach_refcount_tree(struct inode *inode, > > ocfs2_init_dealloc_ctxt(&dealloc); > >- if (!(oi->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL)) { > >+ if (!ocfs2_is_refcount_inode(inode)) { > > ret = ocfs2_create_refcount_tree(inode, di_bh); > > if (ret) { > > mlog_errno(ret); > >diff --git a/fs/ocfs2/refcounttree.h b/fs/ocfs2/refcounttree.h > >index 6422bbc..553edfb 100644 > >--- a/fs/ocfs2/refcounttree.h > >+++ b/fs/ocfs2/refcounttree.h > >@@ -17,6 +17,8 @@ > > #ifndef OCFS2_REFCOUNTTREE_H > > #define OCFS2_REFCOUNTTREE_H > >+bool ocfs2_is_refcount_inode(struct inode *inode); > >+ > > struct ocfs2_refcount_tree { > > struct rb_node rf_node; > > u64 rf_blkno; > > > >-- > >To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in > >the body of a message to majordomo@vger.kernel.org > >More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/ocfs2/refcounttree.c b/fs/ocfs2/refcounttree.c index 1923851..59be8f4 100644 --- a/fs/ocfs2/refcounttree.c +++ b/fs/ocfs2/refcounttree.c @@ -48,6 +48,12 @@ #include <linux/mount.h> #include <linux/posix_acl.h> +/* Does this inode have the reflink flag set? */ +bool ocfs2_is_refcount_inode(struct inode *inode) +{ + return (OCFS2_I(inode)->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL); +} + struct ocfs2_cow_context { struct inode *inode; u32 cow_start; @@ -410,7 +416,7 @@ static int ocfs2_get_refcount_block(struct inode *inode, u64 *ref_blkno) goto out; } - BUG_ON(!(OCFS2_I(inode)->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL)); + BUG_ON(!ocfs2_is_refcount_inode(inode)); di = (struct ocfs2_dinode *)di_bh->b_data; *ref_blkno = le64_to_cpu(di->i_refcount_loc); @@ -570,7 +576,7 @@ static int ocfs2_create_refcount_tree(struct inode *inode, u32 num_got; u64 suballoc_loc, first_blkno; - BUG_ON(oi->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL); + BUG_ON(ocfs2_is_refcount_inode(inode)); trace_ocfs2_create_refcount_tree( (unsigned long long)OCFS2_I(inode)->ip_blkno); @@ -708,7 +714,7 @@ static int ocfs2_set_refcount_tree(struct inode *inode, struct ocfs2_refcount_block *rb; struct ocfs2_refcount_tree *ref_tree; - BUG_ON(oi->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL); + BUG_ON(ocfs2_is_refcount_inode(inode)); ret = ocfs2_lock_refcount_tree(osb, refcount_loc, 1, &ref_tree, &ref_root_bh); @@ -775,7 +781,7 @@ int ocfs2_remove_refcount_tree(struct inode *inode, struct buffer_head *di_bh) u64 blk = 0, bg_blkno = 0, ref_blkno = le64_to_cpu(di->i_refcount_loc); u16 bit = 0; - if (!(oi->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL)) + if (!ocfs2_is_refcount_inode(inode)) return 0; BUG_ON(!ref_blkno); @@ -2299,11 +2305,10 @@ int ocfs2_decrease_refcount(struct inode *inode, { int ret; u64 ref_blkno; - struct ocfs2_inode_info *oi = OCFS2_I(inode); struct buffer_head *ref_root_bh = NULL; struct ocfs2_refcount_tree *tree; - BUG_ON(!(oi->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL)); + BUG_ON(!ocfs2_is_refcount_inode(inode)); ret = ocfs2_get_refcount_block(inode, &ref_blkno); if (ret) { @@ -2533,7 +2538,6 @@ int ocfs2_prepare_refcount_change_for_del(struct inode *inode, int *ref_blocks) { int ret; - struct ocfs2_inode_info *oi = OCFS2_I(inode); struct buffer_head *ref_root_bh = NULL; struct ocfs2_refcount_tree *tree; u64 start_cpos = ocfs2_blocks_to_clusters(inode->i_sb, phys_blkno); @@ -2544,7 +2548,7 @@ int ocfs2_prepare_refcount_change_for_del(struct inode *inode, goto out; } - BUG_ON(!(oi->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL)); + BUG_ON(!ocfs2_is_refcount_inode(inode)); ret = ocfs2_get_refcount_tree(OCFS2_SB(inode->i_sb), refcount_loc, &tree); @@ -3412,14 +3416,13 @@ static int ocfs2_refcount_cow_hunk(struct inode *inode, { int ret; u32 cow_start = 0, cow_len = 0; - struct ocfs2_inode_info *oi = OCFS2_I(inode); struct ocfs2_super *osb = OCFS2_SB(inode->i_sb); struct ocfs2_dinode *di = (struct ocfs2_dinode *)di_bh->b_data; struct buffer_head *ref_root_bh = NULL; struct ocfs2_refcount_tree *ref_tree; struct ocfs2_cow_context *context = NULL; - BUG_ON(!(oi->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL)); + BUG_ON(!ocfs2_is_refcount_inode(inode)); ret = ocfs2_refcount_cal_cow_clusters(inode, &di->id2.i_list, cpos, write_len, max_cpos, @@ -3629,11 +3632,10 @@ int ocfs2_refcount_cow_xattr(struct inode *inode, { int ret; struct ocfs2_xattr_value_root *xv = vb->vb_xv; - struct ocfs2_inode_info *oi = OCFS2_I(inode); struct ocfs2_cow_context *context = NULL; u32 cow_start, cow_len; - BUG_ON(!(oi->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL)); + BUG_ON(!ocfs2_is_refcount_inode(inode)); ret = ocfs2_refcount_cal_cow_clusters(inode, &xv->xr_list, cpos, write_len, UINT_MAX, @@ -3807,7 +3809,7 @@ static int ocfs2_attach_refcount_tree(struct inode *inode, ocfs2_init_dealloc_ctxt(&dealloc); - if (!(oi->ip_dyn_features & OCFS2_HAS_REFCOUNT_FL)) { + if (!ocfs2_is_refcount_inode(inode)) { ret = ocfs2_create_refcount_tree(inode, di_bh); if (ret) { mlog_errno(ret); diff --git a/fs/ocfs2/refcounttree.h b/fs/ocfs2/refcounttree.h index 6422bbc..553edfb 100644 --- a/fs/ocfs2/refcounttree.h +++ b/fs/ocfs2/refcounttree.h @@ -17,6 +17,8 @@ #ifndef OCFS2_REFCOUNTTREE_H #define OCFS2_REFCOUNTTREE_H +bool ocfs2_is_refcount_inode(struct inode *inode); + struct ocfs2_refcount_tree { struct rb_node rf_node; u64 rf_blkno;
Replace the open-coded inode refcount flag test with a helper function to reduce the potential for bugs. Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> --- fs/ocfs2/refcounttree.c | 28 +++++++++++++++------------- fs/ocfs2/refcounttree.h | 2 ++ 2 files changed, 17 insertions(+), 13 deletions(-)