Thank you for your comments, I will revise it and resend later. Regards, Zhu Yanhai
2009/7/28 Ryusuke Konishi <[email protected]>: > 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
