2009-09-22 11:07:43

by Al Viro

[permalink] [raw]
Subject: Re: [PATCH 3/4] Do not allow umounting of frozen filesystems

On Thu, Aug 27, 2009 at 11:06:07PM +0900, Fernando Luis V?zquez Cao wrote:
> Instead of making umount users wait until the filesystem is
> unfreezed return EBUSY, which is very convenient in HA
> configurations.
>
> This could have been implemented at a lower level but it would
> require considerable plumbing in functions such as release_mounts
> which do not return errors.

> + if (sb->s_bdev != NULL) {
> + mutex_lock(&sb->s_bdev->bd_fsfreeze_mutex);
> + if (sb->s_frozen != SB_UNFROZEN) {
> + mutex_unlock(&sb->s_bdev->bd_fsfreeze_mutex);
> + return -EBUSY;
> + }
> + }

NAK. First of all, it _partially_ breaks umount -l for no good reason.
If the first fs on the mountpoint is frozen, we fail; if it's deeper
we succeed just fine (and delay actual fs shutdown until the thaw).

As far as I can see, the real problem is that fsthaw ioctl has braindead
API; it takes some opened file on fs in question. Why not do a bdev
ioctl instead? Then we could let umount go ahead just fine, leaving
fs frozen (and not shut down until it thaws). And whoever does thaw
(via bdev ioctl) will automatically trigger the actual fs shutdown.
Just with Christoph's pair of patches...

IOW, I'd rather add two new ioctls (check if frozen/thaw), both by
bdev. On top of the first two patches in this set.

Comments?


2009-09-22 15:57:38

by Eric Sandeen

[permalink] [raw]
Subject: Re: [PATCH 3/4] Do not allow umounting of frozen filesystems

Al Viro wrote:
...

> IOW, I'd rather add two new ioctls (check if frozen/thaw), both by
> bdev. On top of the first two patches in this set.

> Comments?

The check ioctls would be very very useful, IMHO. Many filesystem tools
refuse to operate on a mounted filesystem, but in some cases it'd be
safe to do readonly operations on a frozen, mounted fs.

-Eric

Subject: Re: [PATCH 3/4] Do not allow umounting of frozen filesystems

Al Viro さんは書きました:
> On Thu, Aug 27, 2009 at 11:06:07PM +0900, Fernando Luis V?zquez Cao wrote:
>
>> Instead of making umount users wait until the filesystem is
>> unfreezed return EBUSY, which is very convenient in HA
>> configurations.
>>
>> This could have been implemented at a lower level but it would
>> require considerable plumbing in functions such as release_mounts
>> which do not return errors.
>>
>
>
>> + if (sb->s_bdev != NULL) {
>> + mutex_lock(&sb->s_bdev->bd_fsfreeze_mutex);
>> + if (sb->s_frozen != SB_UNFROZEN) {
>> + mutex_unlock(&sb->s_bdev->bd_fsfreeze_mutex);
>> + return -EBUSY;
>> + }
>> + }
>>
>
> NAK. First of all, it _partially_ breaks umount -l for no good reason.
> If the first fs on the mountpoint is frozen, we fail; if it's deeper
> we succeed just fine (and delay actual fs shutdown until the thaw).
>
> As far as I can see, the real problem is that fsthaw ioctl has braindead
> API; it takes some opened file on fs in question. Why not do a bdev
> ioctl instead? Then we could let umount go ahead just fine, leaving
> fs frozen (and not shut down until it thaws). And whoever does thaw
> (via bdev ioctl) will automatically trigger the actual fs shutdown.
> Just with Christoph's pair of patches...
>

I basically agree with you. The current API creates a lot of locking
issues that could be tackled
more cleanly with the bdev ioctls you suggest.

> IOW, I'd rather add two new ioctls (check if frozen/thaw), both by
> bdev. On top of the first two patches in this set.
>

I am happy to see you would welcome a check ioctl.

If there is consensus on the bdev ioctl approach I could send patches.

Thanks,

Fernando

Subject: Re: [PATCH 3/4] Do not allow umounting of frozen filesystems

Eric Sandeen さんは書きました:
> Al Viro wrote:
> ...
>
>
>> IOW, I'd rather add two new ioctls (check if frozen/thaw), both by
>> bdev. On top of the first two patches in this set.
>>
>
>
>> Comments?
>>
>
> The check ioctls would be very very useful, IMHO. Many filesystem tools
> refuse to operate on a mounted filesystem, but in some cases it'd be
> safe to do readonly operations on a frozen, mounted fs.
>

Yep, and HA software would benefit from the check ioctls too.

- Fernando