[PATCH] vfs: try to unblock evpoll if mounted filesystem is RDONLY

[PATCH] vfs: try to unblock evpoll if mounted filesystem is RDONLY

[Hui Wang]

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@gmail.com>
---
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);
 	}
 
 	return dget(s->s_root);
-- 
1.8.1.2

Re: [PATCH] vfs: try to unblock evpoll if mounted filesystem is RDONLY

[Tejun Heo]

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@gmail.com>
> ---
> 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?

Thanks.

-- 
tejun

Re: [PATCH] vfs: try to unblock evpoll if mounted filesystem is RDONLY

[Jan Kara]

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@gmail.com>
> > ---
> > 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?

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH] vfs: try to unblock evpoll if mounted filesystem is RDONLY

[Hui Wang]

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@gmail.com>
>>> ---
>>> 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?
>
> 								Honza
Hi Honza,

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.


Accutally this problem is very easily to be reproduced, if you use
any linux distribution with kernel equals or is above linux-3.0, you
can insert a DVD-RW/CD-RW which has a iso9660 filesystem, the desktop
auto-mounter daemon will automatically mount this disc, you can see
this mount is readonly through executing mount command in the
terminal, then you can push the eject button on the disc tray, you
will see that button doesn't work anymore.

Re: [PATCH] vfs: try to unblock evpoll if mounted filesystem is RDONLY

[Jan Kara]

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@gmail.com>
> >>>---
> >>>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@suse.cz>
SUSE Labs, CR

Re: [PATCH] vfs: try to unblock evpoll if mounted filesystem is RDONLY

[Hui Wang]

On 07/25/2013 02:39 AM, Jan Kara wrote:
> 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
[...]
>>>>>      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
Your analysis is reasonable, the iso9660 is easier to be handled than udf.

I also think move the relating code to fs/block_dev.c is a good idea, 
and i am very
glad this issue can raise your attention, looking forward a perfect 
solution for this
issue from you.

Thanks,
Hui.

Re: [PATCH] vfs: try to unblock evpoll if mounted filesystem is RDONLY

[Jan Kara]

On Thu 25-07-13 11:24:00, Hui Wang wrote:
> On 07/25/2013 02:39 AM, Jan Kara wrote:
> >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
> [...]
> >>>>>     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.
> >
> Your analysis is reasonable, the iso9660 is easier to be handled than udf.
> 
> I also think move the relating code to fs/block_dev.c is a good idea, and
> i am very glad this issue can raise your attention, looking forward a
> perfect solution for this issue from you.
  Actually, I've now got idea that seems technically even better to me.
When we are asked to RW mount in case filesystem cannot really do RW, we
return -EROFS instead of silently falling back to RO mount. Userspace
mount(8) command will try again with MS_RDONLY set so things should still
work OK and the problems with eject button would be solved. I'll try to
code that up.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH] vfs: try to unblock evpoll if mounted filesystem is RDONLY

[Hui Wang]

On 07/25/2013 06:03 PM, Jan Kara wrote:
> On Thu 25-07-13 11:24:00, Hui Wang wrote:
>> On 07/25/2013 02:39 AM, Jan Kara wrote:
>>> 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
>> [...]
>>>>>>>      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.
>>>
>> Your analysis is reasonable, the iso9660 is easier to be handled than udf.
>>
>> I also think move the relating code to fs/block_dev.c is a good idea, and
>> i am very glad this issue can raise your attention, looking forward a
>> perfect solution for this issue from you.
>    Actually, I've now got idea that seems technically even better to me.
> When we are asked to RW mount in case filesystem cannot really do RW, we
> return -EROFS instead of silently falling back to RO mount. Userspace
> mount(8) command will try again with MS_RDONLY set so things should still
> work OK and the problems with eject button would be solved. I'll try to
> code that up.
>
> 								Honza
Sounds a better idea. Right now, if we mount the DVD-R without readonly, 
the mount will
try RW mount first, but it will fail since the media is readonly, then 
the mount(8) command will
try again with MS_RDONLY. So your design will work similarly to this 
case, the difference is
filesystem is readonly, the first time mounting will fail from fs.

Hope to see your patch soon. :-)

Regards,
Hui.

Re: [PATCH] vfs: try to unblock evpoll if mounted filesystem is RDONLY

[Tejun Heo]

Hello, Jan.

On Thu, Jul 25, 2013 at 12:03:58PM +0200, Jan Kara wrote:
> When we are asked to RW mount in case filesystem cannot really do RW, we
> return -EROFS instead of silently falling back to RO mount. Userspace
> mount(8) command will try again with MS_RDONLY set so things should still
> work OK and the problems with eject button would be solved. I'll try to
> code that up.

Hmmm.... while it is neat, it is very visible to userland and I'm a
bit concerned that we might break some obscure tool which isn't using
the standard mount.  Not sure about udf but for iso9660, which is
vastly more common anyway, just always setting MS_RDONLY seems like
the better solution.

Thanks.

-- 
tejun

Re: [PATCH] vfs: try to unblock evpoll if mounted filesystem is RDONLY

[Jan Kara]

On Thu 25-07-13 11:33:35, Tejun Heo wrote:
> Hello, Jan.
> 
> On Thu, Jul 25, 2013 at 12:03:58PM +0200, Jan Kara wrote:
> > When we are asked to RW mount in case filesystem cannot really do RW, we
> > return -EROFS instead of silently falling back to RO mount. Userspace
> > mount(8) command will try again with MS_RDONLY set so things should still
> > work OK and the problems with eject button would be solved. I'll try to
> > code that up.
> 
> Hmmm.... while it is neat, it is very visible to userland and I'm a
> bit concerned that we might break some obscure tool which isn't using
> the standard mount.  Not sure about udf but for iso9660, which is
> vastly more common anyway, just always setting MS_RDONLY seems like
> the better solution.
  Well, the trick is that if the media is not writeable, mount without
MS_RDONLY will fail even currently (tried that in practice, also see the
check in blkdev_get_by_path()). So such tool should handle that case even
currently. The only somewhat realistic case I'm aware of is if someone uses
UDF on e.g. USB stick, it is created so that Linux doesn't support writing
to it, and then some tool would be mounting it directly via syscall and would
not handle the read-only media case right. But I don't find this likely so
I think it would be worth trying out this approach before we try playing
tricks with dropping write access... Hmm?

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH] vfs: try to unblock evpoll if mounted filesystem is RDONLY

[Tejun Heo]

Hello, Jan.

On Thu, Jul 25, 2013 at 07:27:23PM +0200, Jan Kara wrote:
> MS_RDONLY will fail even currently (tried that in practice, also see the
> check in blkdev_get_by_path()). So such tool should handle that case even
> currently. The only somewhat realistic case I'm aware of is if someone uses
> UDF on e.g. USB stick, it is created so that Linux doesn't support writing
> to it, and then some tool would be mounting it directly via syscall and would
> not handle the read-only media case right. But I don't find this likely so
> I think it would be worth trying out this approach before we try playing
> tricks with dropping write access... Hmm?

I see.  Yeah, if it's highly unlikely to be noticeable, I think it's a
neat approach.  No objection from me.

Thanks!

-- 
tejun