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@xxxxxxxxxx>
> >---
> > 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@xxxxxxxxxxxxxxx
> >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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
