Re: [PATCH 2/2] fat (exportfs): reconnect file handles to evicted inodes/dentries

[OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>]

"Steven J. Magnani" <steve@digidescorp.com> writes:

> This patch adds code to support reconstruction of evicted inodes.
> We walk the on-disk structures where necessary to fill in information
> not available in the NFS file handle.
>
> One important point is that when reconstructing an inode, in order to avoid the
> *client* declaring ESTALE we have to ensure that the NFS file handle of the
> reconstruction is identical to that of the original. 

Sorry, I didn't review fully yet though.

We would like to add the comment of [0/2] explanation here. Here is
missing the explanation of the problem.

And the codes of nfs support became larger, and I think we should give
the place for it. I.e. we would like to add new file (maybe, export.c,
nfs.c, or something), move nfs code into it.

With it, we don't need to add "NFS ..." annotation in the comment, and
it would make more clear.

And can you add explanation the test of this? What were tested?

> +/**
> + * NFS helper: retrieve the name of an anonymous (disconnected) child using
> + * its i_pos or i_logstart and knowledge of its parent
> + *
> + * Returns 0 on success, -ENOENT if child cannot be found,
> + * -ENOMEM on malloc failure
> + */
> +int fat_get_name(struct dentry *parent, char *name,
> +		 struct dentry *child)
> +{
> +	struct super_block *sb = parent->d_inode->i_sb;
> +
> +	loff_t child_loc = MSDOS_I(child->d_inode)->i_pos;
> +	int err;
> +
> +	lock_super(sb);
> +
> +	if (child_loc) {
> +		err = fat_name_for_ipos(parent->d_inode, name, NAME_MAX+1,
> +					child_loc);
> +	} else {
> +		child_loc = MSDOS_I(child->d_inode)->i_logstart;
> +		err = fat_name_for_logstart(parent->d_inode, name, NAME_MAX+1,
> +					    child_loc);
> +	}
> +
> +	unlock_super(sb);
> +	return err;
> +}

Please don't add new lock_super() usage if it is not necessary. Almost
all of lock_super() just replaced lock_kernel() usage. It rather should
be removed in future.  Probably, this should use inode->i_mutex instead.

BTW, the above issue is same with all of directory read.

And although this is using i_pos, is there no possibility to be passed
the detached inode (i.e. open but unlinked inode, i_pos == 0)?

What happens in the case of detached inode?

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>