Hi!
On Mon, 27 Jul 2009 17:26:58 +0800, Zhu Yanhai wrote:
> Below inconsistent error messages will be reported without this patch.
>
> [r...@zyh-fedora ~]# mount -t nilfs2 /dev/loop0 ./mount1
> mount.nilfs2: WARNING! - The NILFS on-disk format may change at any time.
> mount.nilfs2: WARNING! - Do not place critical data on a NILFS filesystem.
> [r...@zyh-fedora ~]# lscp
> CNO DATE TIME MODE FLG NBLKINC ICNT
> 1 2009-07-26 16:14:26 cp - 11 3
> 2 2009-07-26 16:16:12 cp - 637 144
> 3 2009-07-26 16:16:47 cp - 17 143
> 4 2009-07-26 16:16:52 cp - 16 142
> 5 2009-07-26 16:16:58 cp - 11 142
> 6 2009-07-26 16:17:08 cp - 18 141
> [r...@zyh-fedora ~]# mount -t nilfs2 -r -o cp=10 /dev/loop0 ./mount2
> mount.nilfs2: Error while mounting /dev/loop0 on ./mount2: Invalid argument
> [r...@zyh-fedora ~]# mount -t nilfs2 -r -o cp=20 /dev/loop0 ./mount2
> mount.nilfs2: Error while mounting /dev/loop0 on ./mount2: Invalid argument
> [r...@zyh-fedora ~]# mount -t nilfs2 -r -o cp=30 /dev/loop0 ./mount2
> mount.nilfs2: Error while mounting /dev/loop0 on ./mount2: No such
> file or directory
> [r...@zyh-fedora ~]# mount -t nilfs2 -r -o cp=40 /dev/loop0 ./mount2
> mount.nilfs2: Error while mounting /dev/loop0 on ./mount2: No such
> file or directory
> [r...@zyh-fedora ~]# mount -t nilfs2 -r -o cp=25 /dev/loop0 ./mount2
> mount.nilfs2: Error while mounting /dev/loop0 on ./mount2: No such
> file or directory
> [r...@zyh-fedora ~]# mount -t nilfs2 -r -o cp=21 /dev/loop0 ./mount2
> mount.nilfs2: Error while mounting /dev/loop0 on ./mount2: No such
> file or directory
Thank you for pointing out the inconsistency.
Your patch noticed me that a few problems exist around the
nilfs_cpfile_is_snapshot() function:
1) For invalid checkpoints it should return an ENOENT error, but this
check is missing; a test with nilfs_checkpoint_invalid() should
come before the nilfs_checkpoint_snapshot() test.
2) If nilfs_cpfile_is_snapshot() returned an ENOENT error,
nilfs_fill_super() should convert it to -EINVAL, but this is
missing too. (the ENOENT code is for internal use)
3) Snapshots can be mounted concurrently with a writable mount. So,
the call site of nilfs_cpfile_is_snapshot() must be protected from
segment writer. I mean it should hold read lock for
nilfs->ns_segctor_sem during the call-out like:
down_read(&nilfs->ns_segctor_sem);
err = nilfs_cpfile_is_snapshot(nilfs->ns_cpfile, sbi->s_snapshot_cno);
up_read(&nilfs->ns_segctor_sem);
This is needed even to protect the use of nilfs_mdt_cno().
Then, (1) and (2) would fix the inconsistency problem.
But I think your patch is still needed to exclude the next checkpoint
for snapshot mount.
A little bit problematic.. up to four fixes.
I would appreciate it if you could rewrite patches taking the above
into account.
Thanks in advance,
Ryusuke Konishi
> 2009/7/27 Zhu Yanhai <[email protected]>:
> > nilfs2: Don't load/check cp block if specified cno is larger than the
> > largest exist one.
> > nilfs2 would load invalid cp block, and report random inconsistent error
> > message under this situation before.
> >
> > Signed-off-by: Zhu Yanhai <[email protected]>
> >
> > ---
> > fs/nilfs2/cpfile.c | 7 ++++---
> > 1 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/nilfs2/cpfile.c b/fs/nilfs2/cpfile.c
> > index aec942c..43978a9 100644
> > --- a/fs/nilfs2/cpfile.c
> > +++ b/fs/nilfs2/cpfile.c
> > @@ -814,9 +814,10 @@ int nilfs_cpfile_is_snapshot(struct inode *cpfile,
> > __u64 cno)
> > struct nilfs_checkpoint *cp;
> > void *kaddr;
> > int ret;
> > -
> > - if (cno == 0)
> > - return -ENOENT; /* checkpoint number 0 is invalid */
> > +
> > + /* return ENOENT if cno is invalid. */
> > + if (cno == 0 || cno >= nilfs_mdt_cno(cpfile))
> > + return -ENOENT;
> > down_read(&NILFS_MDT(cpfile)->mi_sem);
> >
> > ret = nilfs_cpfile_get_checkpoint_block(cpfile, cno, 0, &bh);
> > --
> > 1.6.2.2
> >
_______________________________________________
users mailing list
[email protected]
https://www.nilfs.org/mailman/listinfo/users
