Re: [NFS] [PATCH 001 of 8] knfsd: Add nfs-export support to tmpfs

[Neil Brown <neilb@suse.de>]

On Thursday September 28, akpm@osdl.org wrote:
> On Fri, 29 Sep 2006 13:08:39 +1000
> NeilBrown <neilb@suse.de> wrote:
> 
> > +static int shmem_encode_fh(struct dentry *dentry, __u32 *fh, int *len, int connectable)
> > +{
> > +	struct inode *inode = dentry->d_inode;
> > +
> > +	if (*len < 2)
> > +		return 255;
> > +
> > +	if (hlist_unhashed(&inode->i_hash)) {
> > +		/* Unfortunately insert_inode_hash is not idempotent,
> > +		 * so as we hash inodes here rather than at creation
> > +		 * time, we need a lock to ensure we only try
> > +		 * to do it once
> > +		 */
> > +		static DEFINE_SPINLOCK(lock);
> > +		spin_lock(&lock);
> > +		if (hlist_unhashed(&inode->i_hash))
> > +			insert_inode_hash(inode);
> > +		spin_unlock(&lock);
> > +	}
> 
> This looks fishy.
> 
> How do we get two callers in here at the same time for the same inode?

Probably not very easily.
But imagine a file has two hard links in different directories.
And two clients issue LOOKUP requests, one for each link.
They could conceivably be processed at exactly the same time and so
shmem_encode_fh could be running on two different CPU's at the same
time for the same inode.

> 
> Why don't other filesystems have the same problem?
> 

Because most filesystems that hash their inodes do so at the point
where the 'struct inode' is initialised, and that has suitable locking
(I_NEW).  Here in shmem, we are hashing the inode later, the first
time we need an NFS file handle for it.  We no longer have I_NEW to
ensure only one thread tries to add it to the hash table.

The comment tries to explain this, but obviously isn't completely
successful.

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