Re: [PATCH]nilfs2: Don't load/check invalid cno

[Ryusuke Konishi <ryusuke-sG5X7nlA6pw@public.gmane.org>]

Hi!
On Mon, 27 Jul 2009 17:26:58 +0800, Zhu Yanhai wrote:
> Below inconsistent error messages will be reported without this patch.
> 
> [root@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.
> [root@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
> [root@zyh-fedora ~]# mount -t nilfs2 -r -o cp=10 /dev/loop0 ./mount2
> mount.nilfs2: Error while mounting /dev/loop0 on ./mount2: Invalid argument
> [root@zyh-fedora ~]# mount -t nilfs2 -r -o cp=20 /dev/loop0 ./mount2
> mount.nilfs2: Error while mounting /dev/loop0 on ./mount2: Invalid argument
> [root@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
> [root@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
> [root@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
> [root@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 <zhu.yanhai-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> > 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 <zhu.yanhai-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >
> > ---
> >  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
> >