From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from cn.fujitsu.com ([59.151.112.132]:35094 "EHLO heian.cn.fujitsu.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1753153AbbA3Bo2 convert rfc822-to-8bit (ORCPT ); Thu, 29 Jan 2015 20:44:28 -0500 Message-ID: <54CAE1E3.1040406@cn.fujitsu.com> Date: Fri, 30 Jan 2015 09:44:03 +0800 From: Qu Wenruo MIME-Version: 1.0 To: Miao Xie , CC: linux-fsdevel Subject: Re: [PATCH RESEND v4 6/8] vfs: Add get_vfsmount_sb() function to get vfsmount from a given sb. References: <1422498281-20493-1-git-send-email-quwenruo@cn.fujitsu.com> <1422498281-20493-7-git-send-email-quwenruo@cn.fujitsu.com> <54CAD5DC.2060603@huawei.com> In-Reply-To: <54CAD5DC.2060603@huawei.com> Content-Type: text/plain; charset="utf-8"; format=flowed Sender: linux-btrfs-owner@vger.kernel.org List-ID: -------- Original Message -------- Subject: Re: [PATCH RESEND v4 6/8] vfs: Add get_vfsmount_sb() function to get vfsmount from a given sb. From: Miao Xie To: Qu Wenruo , Date: 2015年01月30日 08:52 > On Thu, 29 Jan 2015 10:24:39 +0800, Qu Wenruo wrote: >> There are sysfs interfaces in some fs, only btrfs yet, which will modify >> on-disk data. >> Unlike normal file operation routine we can use mnt_want_write_file() to >> protect the operation, change through sysfs won't to be binded to any file >> in the filesystem. >> So we can only extract the first vfsmount of a superblock and pass it to >> mnt_want_write() to do the protection. > This method is wrong, becasue one fs may be mounted on the multi places > at the same time, someone is R/O, someone is R/W, you may get a R/O and > fail to get the write permission. This shouldn't happen. If someone is ro, the whole fs should be ro, right? You can mount a device which is already mounted as rw to other point as ro, and remount a mount point to ro will also cause all other mount point to ro. So I didn't see the problem here. > > I think you do label/feature change by sysfs interface by the following way > > btrfs_sysfs_change_XXXX() > { > /* Use trylock to avoid the race with umount */ > if(!mutex_trylock(&sb->s_umount)) > return -EBUSY; > > check R/O and FREEZE > > mutex_unlock(&sb->s_umount); > } This looks better since it not introduce changes to VFS. Thanks, Qu > > Thanks > Miao > >> Cc: linux-fsdevel >> Signed-off-by: Qu Wenruo >> --- >> fs/namespace.c | 25 +++++++++++++++++++++++++ >> include/linux/mount.h | 1 + >> 2 files changed, 26 insertions(+) >> >> diff --git a/fs/namespace.c b/fs/namespace.c >> index cd1e968..5a16a62 100644 >> --- a/fs/namespace.c >> +++ b/fs/namespace.c >> @@ -1105,6 +1105,31 @@ struct vfsmount *mntget(struct vfsmount *mnt) >> } >> EXPORT_SYMBOL(mntget); >> >> +/* >> + * get a vfsmount from a given sb >> + * >> + * This is especially used for case where change fs' sysfs interface >> + * will lead to a write, e.g. Change label through sysfs in btrfs. >> + * So vfs can get a vfsmount and then use mnt_want_write() to protect. >> + */ >> +struct vfsmount *get_vfsmount_sb(struct super_block *sb) >> +{ >> + struct vfsmount *ret_vfs = NULL; >> + struct mount *mnt; >> + int ret = 0; >> + >> + lock_mount_hash(); >> + if (list_empty(&sb->s_mounts)) >> + goto out; >> + mnt = list_entry(sb->s_mounts.next, struct mount, mnt_instance); >> + ret_vfs = &mnt->mnt; >> + ret_vfs = mntget(ret_vfs); >> +out: >> + unlock_mount_hash(); >> + return ret_vfs; >> +} >> +EXPORT_SYMBOL(get_vfsmount_sb); >> + >> struct vfsmount *mnt_clone_internal(struct path *path) >> { >> struct mount *p; >> diff --git a/include/linux/mount.h b/include/linux/mount.h >> index c2c561d..cf1b0f5 100644 >> --- a/include/linux/mount.h >> +++ b/include/linux/mount.h >> @@ -79,6 +79,7 @@ extern void mnt_drop_write_file(struct file *file); >> extern void mntput(struct vfsmount *mnt); >> extern struct vfsmount *mntget(struct vfsmount *mnt); >> extern struct vfsmount *mnt_clone_internal(struct path *path); >> +extern struct vfsmount *get_vfsmount_sb(struct super_block *sb); >> extern int __mnt_is_readonly(struct vfsmount *mnt); >> >> struct path; >> From mboxrd@z Thu Jan 1 00:00:00 1970 From: Qu Wenruo Subject: Re: [PATCH RESEND v4 6/8] vfs: Add get_vfsmount_sb() function to get vfsmount from a given sb. Date: Fri, 30 Jan 2015 09:44:03 +0800 Message-ID: <54CAE1E3.1040406@cn.fujitsu.com> References: <1422498281-20493-1-git-send-email-quwenruo@cn.fujitsu.com> <1422498281-20493-7-git-send-email-quwenruo@cn.fujitsu.com> <54CAD5DC.2060603@huawei.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: linux-fsdevel To: Miao Xie , Return-path: Received: from cn.fujitsu.com ([59.151.112.132]:35094 "EHLO heian.cn.fujitsu.com" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1753153AbbA3Bo2 convert rfc822-to-8bit (ORCPT ); Thu, 29 Jan 2015 20:44:28 -0500 In-Reply-To: <54CAD5DC.2060603@huawei.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: -------- Original Message -------- Subject: Re: [PATCH RESEND v4 6/8] vfs: Add get_vfsmount_sb() function=20 to get vfsmount from a given sb. =46rom: Miao Xie To: Qu Wenruo , Date: 2015=E5=B9=B401=E6=9C=8830=E6=97=A5 08:52 > On Thu, 29 Jan 2015 10:24:39 +0800, Qu Wenruo wrote: >> There are sysfs interfaces in some fs, only btrfs yet, which will mo= dify >> on-disk data. >> Unlike normal file operation routine we can use mnt_want_write_file(= ) to >> protect the operation, change through sysfs won't to be binded to an= y file >> in the filesystem. >> So we can only extract the first vfsmount of a superblock and pass i= t to >> mnt_want_write() to do the protection. > This method is wrong, becasue one fs may be mounted on the multi pla= ces > at the same time, someone is R/O, someone is R/W, you may get a R/O a= nd > fail to get the write permission. This shouldn't happen. If someone is ro, the whole fs should be ro, rig= ht? You can mount a device which is already mounted as rw to other point as= ro, and remount a mount point to ro will also cause all other mount point t= o ro. So I didn't see the problem here. > > I think you do label/feature change by sysfs interface by the followi= ng way > > btrfs_sysfs_change_XXXX() > { > /* Use trylock to avoid the race with umount */ > if(!mutex_trylock(&sb->s_umount)) > return -EBUSY; > > check R/O and FREEZE > > mutex_unlock(&sb->s_umount); > } This looks better since it not introduce changes to VFS. Thanks, Qu > > Thanks > Miao > >> Cc: linux-fsdevel >> Signed-off-by: Qu Wenruo >> --- >> fs/namespace.c | 25 +++++++++++++++++++++++++ >> include/linux/mount.h | 1 + >> 2 files changed, 26 insertions(+) >> >> diff --git a/fs/namespace.c b/fs/namespace.c >> index cd1e968..5a16a62 100644 >> --- a/fs/namespace.c >> +++ b/fs/namespace.c >> @@ -1105,6 +1105,31 @@ struct vfsmount *mntget(struct vfsmount *mnt) >> } >> EXPORT_SYMBOL(mntget); >> =20 >> +/* >> + * get a vfsmount from a given sb >> + * >> + * This is especially used for case where change fs' sysfs interfac= e >> + * will lead to a write, e.g. Change label through sysfs in btrfs. >> + * So vfs can get a vfsmount and then use mnt_want_write() to prote= ct. >> + */ >> +struct vfsmount *get_vfsmount_sb(struct super_block *sb) >> +{ >> + struct vfsmount *ret_vfs =3D NULL; >> + struct mount *mnt; >> + int ret =3D 0; >> + >> + lock_mount_hash(); >> + if (list_empty(&sb->s_mounts)) >> + goto out; >> + mnt =3D list_entry(sb->s_mounts.next, struct mount, mnt_instance); >> + ret_vfs =3D &mnt->mnt; >> + ret_vfs =3D mntget(ret_vfs); >> +out: >> + unlock_mount_hash(); >> + return ret_vfs; >> +} >> +EXPORT_SYMBOL(get_vfsmount_sb); >> + >> struct vfsmount *mnt_clone_internal(struct path *path) >> { >> struct mount *p; >> diff --git a/include/linux/mount.h b/include/linux/mount.h >> index c2c561d..cf1b0f5 100644 >> --- a/include/linux/mount.h >> +++ b/include/linux/mount.h >> @@ -79,6 +79,7 @@ extern void mnt_drop_write_file(struct file *file)= ; >> extern void mntput(struct vfsmount *mnt); >> extern struct vfsmount *mntget(struct vfsmount *mnt); >> extern struct vfsmount *mnt_clone_internal(struct path *path); >> +extern struct vfsmount *get_vfsmount_sb(struct super_block *sb); >> extern int __mnt_is_readonly(struct vfsmount *mnt); >> =20 >> struct path; >> -- 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