* [PATCH]nilfs2: Don't load/check invalid cno
@ 2009-07-27 9:25 Zhu Yanhai
[not found] ` <4A6D7273.5010906-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
0 siblings, 1 reply; 4+ messages in thread
From: Zhu Yanhai @ 2009-07-27 9:25 UTC (permalink / raw)
To: NILFS Users mailing list
Cc: qiang.z.zhang-ral2JQCrhuEAvxtiuMwx3w,
yanhai.zhu-VuQAYsv1563Yd54FQh9/CA
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
^ permalink raw reply related [flat|nested] 4+ messages in thread[parent not found: <4A6D7273.5010906-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* Re: [PATCH]nilfs2: Don't load/check invalid cno [not found] ` <4A6D7273.5010906-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> @ 2009-07-27 9:26 ` Zhu Yanhai [not found] ` <977a2be20907270226s7c7037b5v38830e1ba68cefcb-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 0 siblings, 1 reply; 4+ messages in thread From: Zhu Yanhai @ 2009-07-27 9:26 UTC (permalink / raw) To: NILFS Users mailing list Cc: qiang.z.zhang-ral2JQCrhuEAvxtiuMwx3w, yanhai.zhu-VuQAYsv1563Yd54FQh9/CA 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 2009/7/27 Zhu Yanhai <zhu.yanhai@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@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 > _______________________________________________ users mailing list users@nilfs.org https://www.nilfs.org/mailman/listinfo/users ^ permalink raw reply [flat|nested] 4+ messages in thread
[parent not found: <977a2be20907270226s7c7037b5v38830e1ba68cefcb-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [PATCH]nilfs2: Don't load/check invalid cno [not found] ` <977a2be20907270226s7c7037b5v38830e1ba68cefcb-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> @ 2009-07-27 17:40 ` Ryusuke Konishi [not found] ` <20090728.024035.102951444.ryusuke-sG5X7nlA6pw@public.gmane.org> 0 siblings, 1 reply; 4+ messages in thread From: Ryusuke Konishi @ 2009-07-27 17:40 UTC (permalink / raw) To: users-JrjvKiOkagjYtjvyW6yDsg, zhu.yanhai-Re5JQEeQqe8AvxtiuMwx3w Cc: qiang.z.zhang-ral2JQCrhuEAvxtiuMwx3w, yanhai.zhu-VuQAYsv1563Yd54FQh9/CA 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 > > ^ permalink raw reply [flat|nested] 4+ messages in thread
[parent not found: <20090728.024035.102951444.ryusuke-sG5X7nlA6pw@public.gmane.org>]
* Re: [PATCH]nilfs2: Don't load/check invalid cno [not found] ` <20090728.024035.102951444.ryusuke-sG5X7nlA6pw@public.gmane.org> @ 2009-07-28 0:46 ` Zhu Yanhai 0 siblings, 0 replies; 4+ messages in thread From: Zhu Yanhai @ 2009-07-28 0:46 UTC (permalink / raw) To: Ryusuke Konishi Cc: qiang.z.zhang-ral2JQCrhuEAvxtiuMwx3w, yanhai.zhu-VuQAYsv1563Yd54FQh9/CA, users-JrjvKiOkagjYtjvyW6yDsg Thank you for your comments, I will revise it and resend later. Regards, Zhu Yanhai 2009/7/28 Ryusuke Konishi <ryusuke@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@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@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@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 >> > > _______________________________________________ users mailing list users@nilfs.org https://www.nilfs.org/mailman/listinfo/users ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2009-07-28 0:46 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-07-27 9:25 [PATCH]nilfs2: Don't load/check invalid cno Zhu Yanhai
[not found] ` <4A6D7273.5010906-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2009-07-27 9:26 ` Zhu Yanhai
[not found] ` <977a2be20907270226s7c7037b5v38830e1ba68cefcb-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2009-07-27 17:40 ` Ryusuke Konishi
[not found] ` <20090728.024035.102951444.ryusuke-sG5X7nlA6pw@public.gmane.org>
2009-07-28 0:46 ` Zhu Yanhai
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.