On Wed 24-07-13 18:08:40, Hui Wang wrote:
> On 07/24/2013 05:48 AM, Jan Kara wrote:
> >On Tue 23-07-13 11:45:55, Tejun Heo wrote:
> >>Hello,
> >>
> >>(cc'ing Jan and quoting the whole body for him)
> >>
> >>On Mon, Jul 08, 2013 at 10:02:54AM +0800, Hui Wang wrote:
> >>>When inserting a rw optical disc like a DVD/CD rw disc, and we mount
> >>>it without an explicit ro option, the vfs will block its event poll
> >>>workqueue to protect it from damaging while writing to disc, the direct
> >>>result of the blocking of event poll is to make the eject button can't
> >>>work.
> >>>
> >>>This protection is reasonable when the filesystem on the rw disc is
> >>>also rw. but if the filessytem on the rw disc is ro, e.g. the iso9660
> >>>and udf readonly partition, this protection is a little bit weird and
> >>>unneeded, since most people are going to be curious why the eject
> >>>button can't work while the mount is ro?
> >>>
> >>>To make the eject button work again while the mounted filesystem is ro,
> >>>we should inspect the flags of the filesytem's sb and unblock the evpoll
> >>>conditionally, the code refers to the blkdev_put() in the
> >>>fs/block_dev.c.
> >>>
> >>>Signed-off-by: Hui Wang <jason77.wang@xxxxxxxxx>
> >>>---
> >>>I personally don't know if this is a real defect or not, but this
> >>>issue is reported by a customer of our company, he said from the user
> >>>experience, this is a defect, since no matter the disc is ro or rw,
> >>>the mount is ro, the eject button should work.
> >>>
> >>>so far, all DVD/CD and DVD-R/CD-R follow this rule (mount is ro, eject
> >>>button can work), but DVD/CD rw discs don't, no matter the mount is ro
> >>>or rw, the eject button always can't work. So our finial goal is to make
> >>>the eject button can work while the filesystem on the rw disc is ro and
> >>>the whole mounting is ro.
> >>>
> >>> fs/super.c | 19 +++++++++++++++++++
> >>> 1 file changed, 19 insertions(+)
> >>>
> >>>diff --git a/fs/super.c b/fs/super.c
> >>>index 7465d43..7980602 100644
> >>>--- a/fs/super.c
> >>>+++ b/fs/super.c
> >>>@@ -1011,6 +1011,25 @@ struct dentry *mount_bdev(struct file_system_type *fs_type,
> >>> s->s_flags |= MS_ACTIVE;
> >>> bdev->bd_super = s;
> >>>+
> >>>+ mutex_lock(&bdev->bd_mutex);
> >>>+
> >>>+ if ((s->s_flags & MS_RDONLY) && bdev->bd_write_holder) {
> >>>+ int bd_holders;
> >>>+
> >>>+ bd_holders = bdev->bd_holders;
> >>>+ if (bdev == bdev->bd_contains)
> >>>+ bd_holders -= 2;
> >>>+ else
> >>>+ bd_holders -= 1;
> >>>+
> >>>+ if (!bd_holders) {
> >>>+ disk_unblock_events(bdev->bd_disk);
> >>>+ bdev->bd_write_holder = false;
> >>>+ }
> >>>+ }
> >>>+
> >>>+ mutex_unlock(&bdev->bd_mutex);
> >>While the issue seems legitimate to me, the above seems rather scary
> >>to me. Can't iso9660 and udf just open the device ro when they know
> >>that's all they need?
> > They do that - or better VFS does (see fs/super.c:mount_bdev()). So the
> >question really is why bd_write_holder gets set. Maybe it didn't get
> >cleared from previous RW mount because bd_holders never hit zero? Hui have
> >you found a reason for that?
>
> The bd_write_holder is set in the blkdev_get() of fs/block_dev.c.
> It is intentionally set during the rw optical disc mount process.
> Let's make an example to simulate a rw optical disc mount process:
>
> users insert a DVD-RW disc and execute:
> "mount -t iso9660 /dev/sr0 /mnt/sr0" in the shell
> |
> sys_mount(..., flags, ...) /* with the flag without MS_RDONLY */
> |
> do_mount(..., flags, ...)
> |
> mount_fs(..., flags, ...)
> |
> isofs_mount(..., flags, ...)
> |
> mount_bdev(..., flags, ...)
> Here the flags is still without MS_RDONLY, and following code
> is picked from mount_bdev() and is very important for this
> issue:
>
> if (!(flags & MS_RDONLY))
> mode |= FMODE_WRITE;
>
> bdev = blkdev_get_by_path(dev_name, mode, fs_type);
> |
> blkdev_get(..., mode, ...)
> Since mode is FMODE_WRITE and rw optical disc is a writble block
> device, the bd_write_holder will be set in this function.
>
> The the mount process will go on after that, the mount_bdev() will
> continue to call:
>
> s = sget(fs_type, test_bdev_super, set_bdev_super, flags | MS_NOSEC,
> bdev);
> the super_block got from the iso9660 filesystem is absolutely
> MS_RDONLY, so this mount changes to a readonly mount, but no code to
> change the bd_write_holder back to zero and unblock the event poll.
I see. For iso9660 filesystem it is actually pretty easy to fix since
there we *know* we are going to mount it read only (isofs_mount() could
well add S_RDONLY to the flags passed to mount_bdev() which would fix the
issue). With udf it is more complex as there we could mount it read-write
depending on the device, medium, and filesystem features. So there we
really need to be able to drop write access as you suggested. But maybe I'd
put your code into some function like bdev_drop_write_access() in
fs/block_dev.c so that we don't leak bdev peculiarities into superblock
handling functions.
Honza
--
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
