Thanks Ted. Back when I ported wrapfs and unionfs from 3.10 to 3.11, I was trying to understand the new readdir interface in 3.11. It was easy to port wrapfs’s ->readdir b/c wrapfs is so much simpler. And I looked at ecryptfs’s ->readdir to understand how it’s done in another stackable f/s. When it came to porting unionfs’s ->readdir to 3.11, it took me longer b/c unionfs is more complex: its readdir keeps open struct file’s for all lower dirs, keeps virtual state (offset/cookies/etc.) so it can know which lower branch to resume reading entries from, plus it has to eliminate duplicates, handle whiteouts, and opaque directories. Unionfs’s readdir is so complex, the code is spread over several .c files (file.c, dirhelper.c, dirfops.c, and dirops.c). It looks like I added extra code there.
You helped me a lot to narrow the bug down. Now I have to test it with several other lower f/s’s, run my regressions, etc. to ensure it didn’t break anything else. I have a feeling that the line before iterate_dir is indeed breaking stuff; but the line after (ctx->pos = …) may still be needed in order to preserve unionfs’s own directory offset (esp. for large directories with userland has to issue multiple getdents calls). BTW, any idea why the extra line(s) broke it for ext4 but not for ext2/3/others? Cheers, Erez. On May 7, 2014, at 10:45 PM, Theodore Tso <[email protected]> wrote: > Hey Erez, > > I think I found the problem. While looking at unionfs_readdir, I found some > code which didn't make any sense: > > /* Read starting from where we last left off. */ > offset = vfs_llseek(lower_file, uds->dirpos, SEEK_SET); > if (offset < 0) { > pr_err("unionfs: llseek err 1\n"); > err = offset; > goto out; > } > > #if 0 > lower_file->f_pos = ctx->pos; > #endif > err = iterate_dir(lower_file, &buf.ctx); > #if 0 > ctx->pos = buf.ctx.pos; > #endif > > /* Save the position for when we continue. */ > offset = vfs_llseek(lower_file, 0, SEEK_CUR); > if (offset < 0) { > pr_err("unionfs: llseek err 2\n"); > err = offset; > goto out; > } > uds->dirpos = offset; > > Note the two lines which I surrounded with #if 0 / #endif pairs. It looks > like using vfs_llseek() to set the lower_file->f_pos setting --- which is > good, that's the supported interface. But then there is this second > lower_file->f_pos = ctx->pos line which in fact overrides the vfs_llseek() > operation, and it sets and saves lower_file->f_pos from ctx->pos, which if I > understand things correctly is the unionfs_readdir's f_pos, no? > > Anyway, when I commented out those two lines, it seems to make things work. > > I'm not 100% sure I understand this code completely, so it would be nice if > you could confirm if this "fix" looks sane or not. > > Thanks, > > — Ted […] _______________________________________________ unionfs mailing list: http://unionfs.filesystems.org/ [email protected] http://www.fsl.cs.sunysb.edu/mailman/listinfo/unionfs
