[Ocfs2-devel] [PATCH 1/1] OCFS2: add spin lock when accessing inode->i_nlink.

[Ocfs2-devel] [PATCH 1/1] OCFS2: add spin lock when accessing inode->i_nlink.

[wangang wang]

add spin lock when accessing inode->i_nlink in ocfs2_drop_inode().

the patch is against 1.2 svn.

Signed-off-by: Wengang wang <wen.gang.wang@oracle.com>
--
Index: fs/ocfs2/inode.c
===================================================================
--- fs/ocfs2/inode.c	(revision 3101)
+++ fs/ocfs2/inode.c	(working copy)
@@ -991,10 +991,12 @@
 	/* Testing ip_orphaned_slot here wouldn't work because we may
 	 * not have gotten a delete_inode vote from any other nodes
 	 * yet. */
+	spin_lock(&oi->ip_lock);
 	if (oi->ip_flags & OCFS2_INODE_MAYBE_ORPHANED) {
 		mlog(0, "Inode was orphaned on another node, clearing nlink.\n");
 		inode->i_nlink = 0;
 	}
+	spin_unlock(&oi->ip_lock);
 
 	generic_drop_inode(inode);
 

[Ocfs2-devel] [PATCH 1/1] OCFS2: add spin lock when accessing inode->i_nlink.

[Sunil Mushran]

NAK

Firstly ip_lock does not protect inode. Secondly, the field is
protected by inode_lock. Follow the code:

iput (inode_lock taken) => iput_final ==> ocfs2_drop_inode (inode_lock 
still held).

void iput(struct inode *inode)
{
....
                if (atomic_dec_and_lock(&inode->i_count, &inode_lock))
                        iput_final(inode);
}

static inline void iput_final(struct inode *inode)
{
        struct super_operations *op = inode->i_sb->s_op;
        void (*drop)(struct inode *) = generic_drop_inode;

        if (op && op->drop_inode)
                drop = op->drop_inode;
        drop(inode);
}


wangang wang wrote:
> add spin lock when accessing inode->i_nlink in ocfs2_drop_inode().
>
> the patch is against 1.2 svn.
>
> Signed-off-by: Wengang wang <wen.gang.wang@oracle.com>
> --
> Index: fs/ocfs2/inode.c
> ===================================================================
> --- fs/ocfs2/inode.c	(revision 3101)
> +++ fs/ocfs2/inode.c	(working copy)
> @@ -991,10 +991,12 @@
>  	/* Testing ip_orphaned_slot here wouldn't work because we may
>  	 * not have gotten a delete_inode vote from any other nodes
>  	 * yet. */
> +	spin_lock(&oi->ip_lock);
>  	if (oi->ip_flags & OCFS2_INODE_MAYBE_ORPHANED) {
>  		mlog(0, "Inode was orphaned on another node, clearing nlink.\n");
>  		inode->i_nlink = 0;
>  	}
> +	spin_unlock(&oi->ip_lock);
>  
>  	generic_drop_inode(inode);
>  
>
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel at oss.oracle.com
> http://oss.oracle.com/mailman/listinfo/ocfs2-devel
>   

[Ocfs2-devel] [PATCH 1/1] OCFS2: add spin lock when accessing inode->i_nlink.

[wengang wang]

Sunil and Srini,

Yes, it's protected by the inode_lock. and thanks for your detail.
well, I found a fragment of code in ocfs2_meta_lock_update(),

#ifdef OCFS2_DELETE_INODE_WORKAROUND
        /* We might as well check this here - since the inode is now
         * locked, an up to date view will indicate whether this was
         * never actually orphaned -- i_nlink should be zero for an
         * orphaned inode. */
        spin_lock(&oi->ip_lock);
        if (inode->i_nlink &&
            oi->ip_flags & OCFS2_INODE_MAYBE_ORPHANED) {
                mlog(0, "Inode %"MLFu64": clearing maybe_orphaned flag\n",
                     oi->ip_blkno);
                oi->ip_flags &= ~OCFS2_INODE_MAYBE_ORPHANED;
        }
        spin_unlock(&oi->ip_lock);
#endif

the i_nlink and OCFS2_INODE_MAYBE_ORPHANED flag are checked with the 
protection of ip_lock.
If ip_lock is needed here, I think it's need as well in my patch.

thanks,
wengang.

Sunil Mushran wrote:
> NAK
>
> Firstly ip_lock does not protect inode. Secondly, the field is
> protected by inode_lock. Follow the code:
>
> iput (inode_lock taken) => iput_final ==> ocfs2_drop_inode (inode_lock 
> still held).
>
> void iput(struct inode *inode)
> {
> ....
>                if (atomic_dec_and_lock(&inode->i_count, &inode_lock))
>                        iput_final(inode);
> }
>
> static inline void iput_final(struct inode *inode)
> {
>        struct super_operations *op = inode->i_sb->s_op;
>        void (*drop)(struct inode *) = generic_drop_inode;
>
>        if (op && op->drop_inode)
>                drop = op->drop_inode;
>        drop(inode);
> }
>
>
> wangang wang wrote:
>> add spin lock when accessing inode->i_nlink in ocfs2_drop_inode().
>>
>> the patch is against 1.2 svn.
>>
>> Signed-off-by: Wengang wang <wen.gang.wang@oracle.com>
>> -- 
>> Index: fs/ocfs2/inode.c
>> ===================================================================
>> --- fs/ocfs2/inode.c    (revision 3101)
>> +++ fs/ocfs2/inode.c    (working copy)
>> @@ -991,10 +991,12 @@
>>      /* Testing ip_orphaned_slot here wouldn't work because we may
>>       * not have gotten a delete_inode vote from any other nodes
>>       * yet. */
>> +    spin_lock(&oi->ip_lock);
>>      if (oi->ip_flags & OCFS2_INODE_MAYBE_ORPHANED) {
>>          mlog(0, "Inode was orphaned on another node, clearing 
>> nlink.\n");
>>          inode->i_nlink = 0;
>>      }
>> +    spin_unlock(&oi->ip_lock);
>>  
>>      generic_drop_inode(inode);
>>  
>>
>> _______________________________________________
>> Ocfs2-devel mailing list
>> Ocfs2-devel at oss.oracle.com
>> http://oss.oracle.com/mailman/listinfo/ocfs2-devel
>>   
>

-- 
Wengang Wang
Member of Technical Staff
Oracle Asia R&D Center
Open Source Technologies Development

Tel:      +86 10 8278 6265
Mobile:   +86 13381078925

[Ocfs2-devel] [PATCH 1/1] OCFS2: add spin lock when accessing inode->i_nlink.

[Tao Ma]

Hi wengang,

wengang wang wrote:
> Sunil and Srini,
>
> Yes, it's protected by the inode_lock. and thanks for your detail.
> well, I found a fragment of code in ocfs2_meta_lock_update(),
>
> #ifdef OCFS2_DELETE_INODE_WORKAROUND
>         /* We might as well check this here - since the inode is now
>          * locked, an up to date view will indicate whether this was
>          * never actually orphaned -- i_nlink should be zero for an
>          * orphaned inode. */
>         spin_lock(&oi->ip_lock);
>         if (inode->i_nlink &&
>             oi->ip_flags & OCFS2_INODE_MAYBE_ORPHANED) {
>                 mlog(0, "Inode %"MLFu64": clearing maybe_orphaned flag\n",
>                      oi->ip_blkno);
>                 oi->ip_flags &= ~OCFS2_INODE_MAYBE_ORPHANED;
>         }
>         spin_unlock(&oi->ip_lock);
> #endif
>
> the i_nlink and OCFS2_INODE_MAYBE_ORPHANED flag are checked with the 
> protection of ip_lock.
> If ip_lock is needed here, I think it's need as well in my patch.
>   
ip_lock here is used to protect the set of ip_flags.

Regards,
Tao

[Ocfs2-devel] [PATCH 1/1] OCFS2: add spin lock when accessing inode->i_nlink.

[wengang wang]

Hi,

Tao Ma wrote:
> Hi wengang,
>
> wengang wang wrote:
>> Sunil and Srini,
>>
>> Yes, it's protected by the inode_lock. and thanks for your detail.
>> well, I found a fragment of code in ocfs2_meta_lock_update(),
>>
>> #ifdef OCFS2_DELETE_INODE_WORKAROUND
>>         /* We might as well check this here - since the inode is now
>>          * locked, an up to date view will indicate whether this was
>>          * never actually orphaned -- i_nlink should be zero for an
>>          * orphaned inode. */
>>         spin_lock(&oi->ip_lock);
>>         if (inode->i_nlink &&
>>             oi->ip_flags & OCFS2_INODE_MAYBE_ORPHANED) {
>>                 mlog(0, "Inode %"MLFu64": clearing maybe_orphaned 
>> flag\n",
>>                      oi->ip_blkno);
>>                 oi->ip_flags &= ~OCFS2_INODE_MAYBE_ORPHANED;
>>         }
>>         spin_unlock(&oi->ip_lock);
>> #endif
>>
>> the i_nlink and OCFS2_INODE_MAYBE_ORPHANED flag are checked with the 
>> protection of ip_lock.
>> If ip_lock is needed here, I think it's need as well in my patch.
>>   
> ip_lock here is used to protect the set of ip_flags.
>
my concern is that the two function runs as following in timer order.
process A running in ocfs2_drop_inode() and B running in 
ocfs2_meta_lock_update()

A        if (oi->ip_flags & OCFS2_INODE_MAYBE_ORPHANED) {
C(omment): now inode->i_nlink is not 0.
B        if (inode->i_nlink && oi->ip_flags & OCFS2_INODE_MAYBE_ORPHANED) {
B                mlog(0, "Inode %"MLFu64": clearing maybe_orphaned 
flag\n", oi->ip_blkno);
B                oi->ip_flags &= ~OCFS2_INODE_MAYBE_ORPHANED;
B        }
A                mlog(0, "Inode was orphaned on another node, clearing 
nlink.\n");
A                inode->i_nlink = 0;
A        }

thus, as a result,
1) OCFS2_INODE_MAYBE_ORPHANED is cleared but inode->i_nlink is 0. 
but result we wanted is
2) OCFS2_INODE_MAYBE_ORPHANED is set and i_nlink is 0  or
3) OCFS2_INODE_MAYBE_ORPHANED is cleared and i_nlink is not 0.

well, seems 1) won't happen.
when ocfs2_drop_inode() is running, the inode is with 0 ref count and 
with inode_lock's protect. so other process can't get any ref on this 
inode and so won't run ocfs2_meta_lock_update() on this inode.
so the original code is no problem.
but if it's not in ocfs2_drop_inode(), it's not a good code.

thanks,
wengang.
> Regards,
> Tao
>