Messages in this thread | | | From | Neil Brown <> | Date | Fri, 29 Sep 2006 16:48:37 +1000 | Subject | Re: [NFS] [PATCH 001 of 8] knfsd: Add nfs-export support to tmpfs |
| |
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/
| |