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

[Zhu Yanhai]

Thank you for your comments, I will revise it and resend later.

Regards,
Zhu Yanhai

2009/7/28 Ryusuke Konishi <ryusuke at osrg.net>:
> Hi!
> On Mon, 27 Jul 2009 17:26:58 +0800, Zhu Yanhai wrote:
>> Below inconsistent error messages will be reported without this patch.
>>
>> [root at 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 at 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 at zyh-fedora ~]# mount -t nilfs2 -r -o cp=10 /dev/loop0 ./mount2
>> mount.nilfs2: Error while mounting /dev/loop0 on ./mount2: Invalid argument
>> [root at zyh-fedora ~]# mount -t nilfs2 -r -o cp=20 /dev/loop0 ./mount2
>> mount.nilfs2: Error while mounting /dev/loop0 on ./mount2: Invalid argument
>> [root at 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 at 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 at 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 at 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 at gmail.com>:
>> > 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 at gmail.com>
>> >
>> > ---
>> >  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
>> >
>