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

[Hui Wang <hui.wang@canonical.com>]

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.