Messages in this thread | | | Date | Wed, 04 Jul 2012 13:03:12 -0500 | From | "Steve Magnani" <> | Subject | Re: [PATCH 2/2] fat (exportfs): reconnect file handles to evicted inodes/dentries |
| |
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes: > "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.
> We would like to add the comment of [0/2] explanation here. Here is > missing the explanation of the problem.
OK
> 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.
Sure.
> And can you add explanation the test of this? What were tested?
I set up a memory-limited virtual machine with a 2 GB FAT partition containing a kernel tree (~770 MB, ~40000 files, 9 levels) and did some 'cp -r' and 'ls -lR' operations on it, some overlapping, some not.
> > +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.
I will look into this. My concern was freezing the filesystem while we walk the on-disk structures. Also I developed this patch against 2.6.35 (the Bad Old BKL days) and ported it forward to 3.5.
> 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)?
It is possible, that's why I added code to fall back to using logstart.
I may yet rip out the get_name code. The testing I did before posting the patch seemed to indicate that it was needed - I saw ESTALE errors without get_name support that I did not see with it present. But I've been digging into this some more and I think that was just a coincidence; probably I just generated more extreme memory pressure when testing without get_name. I should know more tomorrow.
Thanks for the quick feedback. Steve
| |