From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Eric Ren <zren@suse.com>
Cc: mfasheh@versity.com, jlbec@evilplan.org,
linux-fsdevel@vger.kernel.org, ocfs2-devel@oss.oracle.com
Subject: Re: [PATCH 1/6] ocfs2: convert inode refcount test to a helper
Date: Thu, 10 Nov 2016 09:51:05 -0800 [thread overview]
Message-ID: <20161110175105.GB16807@birch.djwong.org> (raw)
In-Reply-To: <22f65896-f2c7-fe5d-9822-c9e7f7ffc8b6@suse.com>
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
next prev parent reply other threads:[~2016-11-10 17:51 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-11-09 22:51 [PATCH 0/6] ocfs2: wire up {clone,copy,dedupe}_range Darrick J. Wong
2016-11-09 22:51 ` [PATCH 1/6] ocfs2: convert inode refcount test to a helper Darrick J. Wong
2016-11-10 2:14 ` Eric Ren
2016-11-10 17:51 ` Darrick J. Wong [this message]
2016-11-10 17:52 ` [PATCH v2 " Darrick J. Wong
2016-11-09 22:51 ` [PATCH 2/6] ocfs2: add newlines to some error messages Darrick J. Wong
2016-11-09 22:51 ` [PATCH 3/6] ocfs2: prohibit refcounted swapfiles Darrick J. Wong
2016-11-09 22:51 ` [PATCH 4/6] ocfs2: budget for extent tree splits when adding refcount flag Darrick J. Wong
2016-11-10 9:20 ` [Ocfs2-devel] " Darwin
2016-11-10 17:11 ` Darrick J. Wong
2016-11-11 3:00 ` Darwin
2016-11-09 22:51 ` [PATCH 5/6] ocfs2: don't eat io errors during _dio_end_io_write Darrick J. Wong
2016-11-09 22:51 ` [PATCH 6/6] ocfs2: implement the VFS clone_range, copy_range, and dedupe_range features Darrick J. Wong
2016-11-11 5:49 ` [Ocfs2-devel] " Eric Ren
2016-11-11 6:20 ` Darrick J. Wong
2016-11-11 6:45 ` Eric Ren
2016-11-11 9:01 ` Darrick J. Wong
2016-11-11 14:54 ` [PATCH v2 " Darrick J. Wong
2016-11-09 23:00 ` [PATCH 7/6] xfstests: fix some minor problems testing ocfs2 Darrick J. Wong
2016-11-11 3:15 ` [PATCH 0/6] ocfs2: wire up {clone,copy,dedupe}_range Eric Ren
2016-11-11 15:05 ` Darrick J. Wong
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20161110175105.GB16807@birch.djwong.org \
--to=darrick.wong@oracle.com \
--cc=jlbec@evilplan.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=mfasheh@versity.com \
--cc=ocfs2-devel@oss.oracle.com \
--cc=zren@suse.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).