| Message ID | 1482915207-109005-1-git-send-email-colyli@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
CC'ing MTD On Wed, Dec 28, 2016 at 9:53 AM, Coly Li <colyli@suse.de> wrote: > static struct kmem_cache *romfs_inode_cachep; > @@ -416,8 +417,14 @@ static void romfs_destroy_inode(struct inode *inode) > static int romfs_statfs(struct dentry *dentry, struct kstatfs *buf) > { > struct super_block *sb = dentry->d_sb; > - u64 id = huge_encode_dev(sb->s_bdev->bd_dev); > + u64 id = 0; > > +#ifdef CONFIG_ROMFS_ON_BLOCK > + id = huge_encode_dev(sb->s_bdev->bd_dev); > +#endif > +#ifdef CONFIG_ROMFS_ON_MTD > + id = huge_encode_dev(sb->s_dev); > +#endif How is this supposed to work with CONFIG_ROMFS_BACKED_BY_BOTH=y? > buf->f_type = ROMFS_MAGIC; > buf->f_namelen = ROMFS_MAXFN; > buf->f_bsize = ROMBSIZE; > @@ -489,6 +496,13 @@ static int romfs_fill_super(struct super_block *sb, void *data, int silent) > sb->s_flags |= MS_RDONLY | MS_NOATIME; > sb->s_op = &romfs_super_ops; > > +#ifdef CONFIG_ROMFS_ON_MTD > + /* Use same dev ID from the underlying mtdblock device */ > + if (sb->s_mtd) > + sb->s_dev = MKDEV(MTD_BLOCK_MAJOR, sb->s_mtd->index); > + else > + sb->s_dev = MKDEV(MTD_BLOCK_MAJOR, 0); Hmm, when there is no MTD, s_dev is still equal to mtd0, since mtd0 is ->index of value 0. This seems fishy to me.
On 2016/12/28 下午5:44, Richard Weinberger wrote: > CC'ing MTD > > On Wed, Dec 28, 2016 at 9:53 AM, Coly Li <colyli@suse.de> wrote: >> static struct kmem_cache *romfs_inode_cachep; >> @@ -416,8 +417,14 @@ static void romfs_destroy_inode(struct inode *inode) >> static int romfs_statfs(struct dentry *dentry, struct kstatfs *buf) >> { >> struct super_block *sb = dentry->d_sb; >> - u64 id = huge_encode_dev(sb->s_bdev->bd_dev); >> + u64 id = 0; >> >> +#ifdef CONFIG_ROMFS_ON_BLOCK >> + id = huge_encode_dev(sb->s_bdev->bd_dev); >> +#endif >> +#ifdef CONFIG_ROMFS_ON_MTD >> + id = huge_encode_dev(sb->s_dev); >> +#endif > > How is this supposed to work with CONFIG_ROMFS_BACKED_BY_BOTH=y? Aha! Thanks for telling me this config option. The purpose of f_fsid is to unify a file system volume, if CONFIG_ROMFS_BACKED_BY_BOTH=y, it can firstly try sb->s_bdev->bd_dev. If sb->s_bdev is NULL, then try sb->s_dev. If both failed, then f_fsid will be 0. > >> buf->f_type = ROMFS_MAGIC; >> buf->f_namelen = ROMFS_MAXFN; >> buf->f_bsize = ROMBSIZE; >> @@ -489,6 +496,13 @@ static int romfs_fill_super(struct super_block *sb, void *data, int silent) >> sb->s_flags |= MS_RDONLY | MS_NOATIME; >> sb->s_op = &romfs_super_ops; >> >> +#ifdef CONFIG_ROMFS_ON_MTD >> + /* Use same dev ID from the underlying mtdblock device */ >> + if (sb->s_mtd) >> + sb->s_dev = MKDEV(MTD_BLOCK_MAJOR, sb->s_mtd->index); >> + else >> + sb->s_dev = MKDEV(MTD_BLOCK_MAJOR, 0); > > Hmm, when there is no MTD, s_dev is still equal to mtd0, since mtd0 is > ->index of > value 0. This seems fishy to me. You are right, if both CONFIG_ROMFS_ON_BLOCK and CONFIG_ROMFS_ON_MTD are defined, here is buggy. Thanks for your review, I will send out another version. Coly -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/romfs/super.c b/fs/romfs/super.c index d0f8a38..a38c07f 100644 --- a/fs/romfs/super.c +++ b/fs/romfs/super.c @@ -74,6 +74,7 @@ #include <linux/highmem.h> #include <linux/pagemap.h> #include <linux/uaccess.h> +#include <linux/major.h> #include "internal.h" static struct kmem_cache *romfs_inode_cachep; @@ -416,8 +417,14 @@ static void romfs_destroy_inode(struct inode *inode) static int romfs_statfs(struct dentry *dentry, struct kstatfs *buf) { struct super_block *sb = dentry->d_sb; - u64 id = huge_encode_dev(sb->s_bdev->bd_dev); + u64 id = 0; +#ifdef CONFIG_ROMFS_ON_BLOCK + id = huge_encode_dev(sb->s_bdev->bd_dev); +#endif +#ifdef CONFIG_ROMFS_ON_MTD + id = huge_encode_dev(sb->s_dev); +#endif buf->f_type = ROMFS_MAGIC; buf->f_namelen = ROMFS_MAXFN; buf->f_bsize = ROMBSIZE; @@ -489,6 +496,13 @@ static int romfs_fill_super(struct super_block *sb, void *data, int silent) sb->s_flags |= MS_RDONLY | MS_NOATIME; sb->s_op = &romfs_super_ops; +#ifdef CONFIG_ROMFS_ON_MTD + /* Use same dev ID from the underlying mtdblock device */ + if (sb->s_mtd) + sb->s_dev = MKDEV(MTD_BLOCK_MAJOR, sb->s_mtd->index); + else + sb->s_dev = MKDEV(MTD_BLOCK_MAJOR, 0); +#endif /* read the image superblock and check it */ rsb = kmalloc(512, GFP_KERNEL); if (!rsb)
'Commit 8a59f5d25265 ("fs/romfs: return f_fsid for statfs(2)")' generates a 64bit id from sb->s_bdev->bd_dev. This is only correct when romfs is defined with CONFIG_ROMFS_ON_BLOCK. If romfs is defined with CONFIG_ROMFS_ON_MTD, sb->s_bdev is NULL, referencing sb->s_bdev->bd_dev will triger an oops. If romfs is built on top of a MTD abstracted block device, this MTD block device has a device ID generated by MTD_BLOCK_MAJOR and mtd index. This patch uses the same ID to generate fsid for the romfs on top of the MTD block device. Generally only one romfs can be built on single MTD block device, this method is enough to identify multiple romfs instances in a computer. If romfs is built on top of a common block device, sb->s_bdev->bd_dev is used to generate the 64bit id, as previous commit did. Signed-off-by: Coly Li <colyli@suse.de> Reported-by: Nong Li <nongli1031@gmail.com> Tested-by: Nong Li <nongli1031@gmail.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: linux-fsdevel@vger.kernel.org --- fs/romfs/super.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)