linux-kernel - Re: linux-next: manual merge of the vfs-scale tree with the v9fs tree

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Wed, 05 Jan 2011 15:05:24 -0800
From:	"Venkateswararao Jujjuri (JV)" <jvrao@...ux.vnet.ibm.com>
To:	Nick Piggin <npiggin@...nel.dk>
CC:	Stephen Rothwell <sfr@...b.auug.org.au>,
	linux-next@...r.kernel.org, linux-kernel@...r.kernel.org,
	"Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>,
	Eric Van Hensbergen <ericvh@...il.com>
Subject: Re: linux-next: manual merge of the vfs-scale tree with the v9fs
 tree

On 1/4/2011 10:44 PM, Nick Piggin wrote:
> On Tue, Jan 04, 2011 at 10:16:07AM -0800, Venkateswararao Jujjuri (JV) wrote:
>> On 1/3/2011 5:40 PM, Stephen Rothwell wrote:
>>> Hi Nick,
>>>
>>> Today's linux-next merge of the vfs-scale tree got a conflict in
>>> fs/9p/vfs_inode.c between commit 3d21652a1d23591e7f0bbbbedae29ce78c2c1113
>>> ("fs/9p: Move dotl inode operations into a seperate file") from the v9fs
>>> tree and various commits from the vfs-scale tree.
>>>
>>> I fixed it up by using the v9fs changes to that file and then applying
>>> the following merge fixup patch (which I can carry as necessary).
>>>
>>> Someone will need to fix this up before one of these trees is merged by
>>> Linus, or to send this merge fix to Linus.
>>>
>>> From: Stephen Rothwell <sfr@...b.auug.org.au>
>>> Date: Tue, 4 Jan 2011 12:33:54 +1100
>>> Subject: [PATCH] v9fs: merge fix for changes in the vfs-scale tree
>>>
>>> Signed-off-by: Stephen Rothwell <sfr@...b.auug.org.au>
>>> ---
>>>  fs/9p/vfs_inode_dotl.c |   22 +++++++++++-----------
>>>  1 files changed, 11 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
>>> index 38d5880..9dd534b 100644
>>> --- a/fs/9p/vfs_inode_dotl.c
>>> +++ b/fs/9p/vfs_inode_dotl.c
>>> @@ -78,11 +78,11 @@ static struct dentry *v9fs_dentry_from_dir_inode(struct inode *inode)
>>>  {
>>>  	struct dentry *dentry;
>>>
>>> -	spin_lock(&dcache_lock);
>>> +	spin_lock(&inode->i_lock);
>>>  	/* Directory should have only one entry. */
>>>  	BUG_ON(S_ISDIR(inode->i_mode) && !list_is_singular(&inode->i_dentry));
>>>  	dentry = list_entry(inode->i_dentry.next, struct dentry, d_alias);
>>> -	spin_unlock(&dcache_lock);
>>> +	spin_unlock(&inode->i_lock);
>>
>> Are we doing away with dcache_lock?
> 
> It's on its last legs...
> 
> 
>> I am not sure if the i_lock can serve the same purpose..but looks like with the
>> current code
>> there may not need any lock around this code. Aneesh/Eric do you guys have any
>> comments?
> 
> Well first of all, why do you say i_lock can't serve the same purpose?

What I mean is i_lock is not equivalent to dcache_lock in generic sense.

> Removing locks is well and good, but if i_lock doesn't work here, then
> I've made a mistake either fudnamentally in the dcache, or with a
> particular pattern that v9fs uses -- either way it has to be understood.
> 

Initially we had a version where we walk up the d_parent to construct
the complete path and corresponding fids for the entire path for 9P purpose.
As we are walking we thought of using big hammer to lock the entire dcache.
Later we used read/write locks to protect race between v9fs_fid_lookup and
rename operations.
Hence I don't think we need to protect this with  dcache_lock.
But having i_lock around this code looks good to me.

> dcache lock removal of course isn't done ad-hoc. These two patches
> specifically are the ones which aim to replace this particular instance
> of locking:

While reading patches below...
> 
> http://git.kernel.org/?p=linux/kernel/git/npiggin/linux-npiggin.git;a=commit;h=5d30c20d47023b95b2a0d4820917dba8ba218d1a

@@ -271,9 +271,11 @@ static struct dentry *v9fs_dentry_from_dir_inode(struct
inode *inode)
        struct dentry *dentry;

        spin_lock(&dcache_lock);
+       spin_lock(&dcache_inode_lock);
        /* Directory should have only one entry. */
        BUG_ON(S_ISDIR(inode->i_mode) && !list_is_singular(&inode->i_dentry));
        dentry = list_entry(inode->i_dentry.next, struct dentry, d_alias);
+       spin_unlock(&dcache_inode_lock);
        spin_unlock(&dcache_lock);
        return dentry;
> http://git.kernel.org/?p=linux/kernel/git/npiggin/linux-npiggin.git;a=commit;h=ef4953a772e04aef8cf5b94a9b70ffbb12b576e2
> 
@@ -277,11 +277,11 @@ static struct dentry *v9fs_dentry_from_dir_inode(struct
inode *inode)
 {
        struct dentry *dentry;

-       spin_lock(&dcache_inode_lock);
+       spin_lock(&inode->i_lock);
        /* Directory should have only one entry. */
        BUG_ON(S_ISDIR(inode->i_mode) && !list_is_singular(&inode->i_dentry));
        dentry = list_entry(inode->i_dentry.next, struct dentry, d_alias);
-       spin_unlock(&dcache_inode_lock);
+       spin_unlock(&inode->i_lock);
        return dentry;
 }

Wondering if there is another patch in between to take out the dcache_lock.

Anyway, Changes in this patch look good to me and sorry for the confusion.

Thanks,
JV
> 
>>>  	return dentry;
>>>  }
>>>
>>> @@ -215,7 +215,7 @@ v9fs_vfs_create_dotl(struct inode *dir, struct dentry *dentry, int omode,
>>>  				err);
>>>  			goto error;
>>>  		}
>>> -		dentry->d_op = &v9fs_cached_dentry_operations;
>>> +		d_set_d_op(dentry, &v9fs_cached_dentry_operations);
>>
>> Assuming that this is a macro to the same operation.. rest of the changes look
>> fine to me.
> 
> Yes it's equivalent.
> 
> Thanks,
> Nick
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/