filesystem freeze: fix sys_umount induced perpetual freeze
Subsystems (2)
RFC PATCH 1/1 filesystem freeze: fix sys_umount induced perpetual freeze
The current locking scheme for filesystem freeze avoids races between freeze_bdev() and do_umount() by taking the s_umount semaphore.
If freeze_bdev() wins the race the process that invoked sys_umount will sleep until thaw_bdev releases the semaphore. Unfortunately, this will never happen because thaw_bdev bails out early the bd_fsfreeze_count check having failed (the count is 0).
The problem is that the block_device that ioctl_fsthaw() passes to thaw_bdev() is not the one that we freezed because before sleeping in deactivate_super() do_umount() released the dentry (dput()) and freed the vfs mount (free_vfsmnt()).
This patch works around this issue by checking the freeze level in do_umount().
---
fs/namespace.c +2 -0
@@ -1033,6 +1033,8 @@ static int do_umount(struct vfsmount *mnif (retval)return retval; +vfs_check_frozen (sb,SB_FREEZE_WRITE ); +/* * Allow userspace to request a mountpoint be expired rather than * unmounting unconditionally. Unmount only happens if:
Quoted 8 lines · Fernando Luis V??zquez Cao
The current locking scheme for filesystem freeze avoids races between
freeze_bdev()anddo_umount()by taking thes_umountsemaphore.If
freeze_bdev()wins the race the process that invokedsys_umountwill sleep untilthaw_bdevreleases the semaphore. Unfortunately, this will never happen becausethaw_bdevbails out early thebd_fsfreeze_countcheck having failed (the count is 0).
Quoted 7 lines
The problem is that the
block_devicethatioctl_fsthaw()passes tothaw_bdev()is not the one that we freezed because before sleeping indeactivate_super()do_umount()released the dentry (dput()) and freed the vfs mount (free_vfsmnt()).This patch works around this issue by checking the freeze level in
do_umount()
This should be solved my my freeze locking rewrite:
http://marc.info/?l=linux-fsdevel&m=124933489118480&w=2 http://marc.info/?l=linux-fsdevel&m=124933491918517&w=2
Quoted 14 lines · Christoph Hellwig
Quoted 13 lines · Fernando Luis V??zquez Cao
The current locking scheme for filesystem freeze avoids races between
freeze_bdev()anddo_umount()by taking thes_umountsemaphore.If
freeze_bdev()wins the race the process that invokedsys_umountwill sleep untilthaw_bdevreleases the semaphore. Unfortunately, this will never happen becausethaw_bdevbails out early thebd_fsfreeze_countcheck having failed (the count is 0).The problem is that the
block_devicethatioctl_fsthaw()passes tothaw_bdev()is not the one that we freezed because before sleeping indeactivate_super()do_umount()released the dentry (dput()) and freed the vfs mount (free_vfsmnt()).
Quoted 8 lines
Quoted 2 lines
This patch works around this issue by checking the freeze level in
do_umount()This should be solved my my freeze locking rewrite:
http://marc.info/?l=linux-fsdevel&m=124933489118480&w=2 http://marc.info/?l=linux-fsdevel&m=124933491918517&w=2
Hi Christoph,
Thank you for the pointers. I gave those patches a spin, but it seems the umount case is not being tackled. I suggest rejecting the umount for frozen filesystems. What do you think?
I will be replying to this email with a forward port of your patches along with my own patches that fix the locking for umount and add a new ioctl to check the freeze state of the filesystem (this is helpful to create clean resource agents for HA solutions).
- Fernando
Quoted 8 lines · Fernando Luis V?zquez Cao
Thank you for the pointers. I gave those patches a spin, but it seems the umount case is not being tackled. I suggest rejecting the umount for frozen filesystems. What do you think?
I will be replying to this email with a forward port of your patches along with my own patches that fix the locking for umount and add a new ioctl to check the freeze state of the filesystem (this is helpful to create clean resource agents for HA solutions).
I think we should just reject the umount for a forzen filesystem with -EBUSY like I do for remount in those patches. I somehow thought I did this but didn't. Is that good for your use case?
Quoted 5 lines · Christoph Hellwig
Quoted 4 lines · Fernando Luis V?zquez Cao
Thank you for the pointers. I gave those patches a spin, but it seems the umount case is not being tackled. I suggest rejecting the umount for frozen filesystems. What do you think?
Quoted 8 lines
Quoted 4 lines
I will be replying to this email with a forward port of your patches along with my own patches that fix the locking for umount and add a new ioctl to check the freeze state of the filesystem (this is helpful to create clean resource agents for HA solutions).
I think we should just reject the umount for a forzen filesystem with
-EBUSYlike I do for remount in those patches. I somehow thought I did this but didn't. Is that good for your use case?
Yes, absolutely. Today I will be sending patches that do just that. Hopefully you will find the new ioctl (ISFROZEN) I am adding helpful too.
Thanks,
Fernando
PATCH 1/4 freeze_bdev: kill bd_mount_sem
In reply to Fernando Luis Vázquez CaoAuthor: Christoph Hellwig <[email protected]>
Subject: [PATCH 1/4] freeze_bdev: kill bd_mount_sem
Now that we have the freeze count there is not much reason for bd_mount_sem anymore. The actual freeze/thaw operations are serialized using the bd_fsfreeze_mutex, and the only other place we take bd_mount_sem is get_sb_bdev which tries to prevent mounting a filesystem while the block device is frozen. Instead of add a check for bd_fsfreeze_count and return -EBUSY if a filesystem is frozen. While that is a change in user visible behaviour a failing mount is much better for this case rather than having the mount process stuck uninterruptible for a long time.
---
fs/block_dev.c +1 -7
@@ -216,8 +216,6 @@ EXPORT_SYMBOL(fsync_bdev);* freeze_bdev -- lock a filesystem and force it into a consistent state * @bdev: blockdevice to lock * -* This takes the block device bd_mount_sem to make sure no new mounts -* happen on bdev until thaw_bdev() is called. * If a superblock is found on this device, we take the s_umount semaphore * on it to make sure nobody unmounts until the snapshot creation is done. * The reference counter (bd_fsfreeze_count) guarantees that only the last
@@ -240,7 +238,6 @@ struct super_block *freeze_bdev(struct b } bdev->bd_fsfreeze_count++; -down (&bdev->bd_mount_sem); sb =get_super (bdev);if (sb && !(sb->s_flags &MS_RDONLY )) { sb->s_frozen =SB_FREEZE_WRITE ;
@@ -260,7 +257,6 @@ struct super_block *freeze_bdev(struct b "VFS:Filesystem freeze failed\n"); sb->s_frozen =SB_UNFROZEN ;drop_super (sb); -up (&bdev->bd_mount_sem); bdev->bd_fsfreeze_count--;mutex_unlock (&bdev->bd_fsfreeze_mutex);return ERR_PTR (error);
@@ -271,7 +267,7 @@ struct super_block *freeze_bdev(struct bsync_blockdev (bdev);mutex_unlock (&bdev->bd_fsfreeze_mutex); -return sb;/* thaw_bdev releases s->s_umount and bd_mount_sem */ +return sb;/* thaw_bdev releases s->s_umount */ }EXPORT_SYMBOL (freeze_bdev);
@@ -321,7 +317,6 @@ int thaw_bdev(struct block_device *bdev,drop_super (sb); } -up (&bdev->bd_mount_sem);mutex_unlock (&bdev->bd_fsfreeze_mutex);return 0 ; }
@@ -431,7 +426,6 @@ static void init_once(void *foo)memset (bdev,0 ,sizeof (*bdev));mutex_init (&bdev->bd_mutex); -sema_init (&bdev->bd_mount_sem,1 );INIT_LIST_HEAD (&bdev->bd_inodes);INIT_LIST_HEAD (&bdev->bd_list);#ifdef CONFIG_SYSFS
fs/super.c +7 -2
@@ -740,9 +740,14 @@ int get_sb_bdev(struct file_system_type* will protect the lockfs code from trying to start a snapshot * while we are mounting */ -down (&bdev->bd_mount_sem); +mutex_lock (&bdev->bd_fsfreeze_mutex); +if (bdev->bd_fsfreeze_count >0 ) { +mutex_unlock (&bdev->bd_fsfreeze_mutex); + error = -EBUSY ; + goto error_bdev; + } s =sget (fs_type, test_bdev_super, set_bdev_super, bdev); -up (&bdev->bd_mount_sem); +mutex_unlock (&bdev->bd_fsfreeze_mutex);if (IS_ERR (s)) goto error_s;
include/linux/fs.h +0 -1
@@ -640,7 +640,6 @@ struct block_device {struct super_block * bd_super;int bd_openers;struct mutex bd_mutex;/* open/close mutex */ -struct semaphore bd_mount_sem;struct list_head bd_inodes;void * bd_holder;int bd_holders;
PATCH 2/4 freeze_bdev: grab active reference to frozen superblocks
In reply to Fernando Luis Vázquez CaoAuthor: Christoph Hellwig <[email protected]>
Subject: [PATCH 2/4] freeze_bdev: grab active reference to frozen superblocks
Currently we held s_umount while a filesystem is frozen, despite that we might return to userspace and unlock it from a different process. Instead grab an active reference to keep the file system busy and add an explicit check for frozen filesystems in remount and reject the remount instead of blocking on s_umount.
Add a new get_active_super helper to super.c for use by freeze_bdev that grabs an active reference to a superblock from a given block device.
---
fs/block_dev.c +70 -58
@@ -230,43 +230,54 @@ struct super_block *freeze_bdev(struct bint error =0 ;mutex_lock (&bdev->bd_fsfreeze_mutex); -if (bdev->bd_fsfreeze_count >0 ) { - bdev->bd_fsfreeze_count++; +if (++bdev->bd_fsfreeze_count >1 ) { +/* +* We don't even need to grab a reference - the first call +* to freeze_bdev grab an active reference and only the last +* thaw_bdev drops it. +*/ sb =get_super (bdev); +drop_super (sb);mutex_unlock (&bdev->bd_fsfreeze_mutex);return sb; } - bdev->bd_fsfreeze_count++; - sb =get_super (bdev); -if (sb && !(sb->s_flags &MS_RDONLY )) { - sb->s_frozen =SB_FREEZE_WRITE ; -smp_wmb (); - -sync_filesystem (sb); - - sb->s_frozen =SB_FREEZE_TRANS ; -smp_wmb (); - -sync_blockdev (sb->s_bdev); - -if (sb->s_op->freeze_fs) { - error = sb->s_op->freeze_fs (sb); -if (error) { -printk (KERN_ERR -"VFS:Filesystem freeze failed\n" ); - sb->s_frozen =SB_UNFROZEN ; -drop_super (sb); - bdev->bd_fsfreeze_count--; -mutex_unlock (&bdev->bd_fsfreeze_mutex); -return ERR_PTR (error); - } + sb =get_active_super (bdev); +if (!sb) + goto out; +if (sb->s_flags &MS_RDONLY ) { +deactivate_locked_super (sb); +mutex_unlock (&bdev->bd_fsfreeze_mutex); +return sb; + } + + sb->s_frozen =SB_FREEZE_WRITE ; +smp_wmb (); + +sync_filesystem (sb); + + sb->s_frozen =SB_FREEZE_TRANS ; +smp_wmb (); + +sync_blockdev (sb->s_bdev); + +if (sb->s_op->freeze_fs) { + error = sb->s_op->freeze_fs (sb); +if (error) { +printk (KERN_ERR +"VFS:Filesystem freeze failed\n" ); + sb->s_frozen =SB_UNFROZEN ; +deactivate_locked_super (sb); + bdev->bd_fsfreeze_count--; +mutex_unlock (&bdev->bd_fsfreeze_mutex); +return ERR_PTR (error); } } +up_write (&sb->s_umount); + out:sync_blockdev (bdev);mutex_unlock (&bdev->bd_fsfreeze_mutex); -return sb;/* thaw_bdev releases s->s_umount */ }EXPORT_SYMBOL (freeze_bdev);
@@ -280,43 +291,44 @@ EXPORT_SYMBOL(freeze_bdev);*/ int thaw_bdev (struct block_device *bdev,struct super_block *sb) { -int error =0 ; +int error = -EINVAL ;mutex_lock (&bdev->bd_fsfreeze_mutex); -if (!bdev->bd_fsfreeze_count) { -mutex_unlock (&bdev->bd_fsfreeze_mutex); -return -EINVAL ; - } +if (!bdev->bd_fsfreeze_count) + goto out_unlock; - bdev->bd_fsfreeze_count--; -if (bdev->bd_fsfreeze_count >0 ) { -if (sb) -drop_super (sb); -mutex_unlock (&bdev->bd_fsfreeze_mutex); -return 0 ; - } - -if (sb) { -BUG_ON (sb->s_bdev != bdev); -if (!(sb->s_flags &MS_RDONLY )) { -if (sb->s_op->unfreeze_fs) { - error = sb->s_op->unfreeze_fs (sb); -if (error) { -printk (KERN_ERR -"VFS:Filesystem thaw failed\n" ); - sb->s_frozen =SB_FREEZE_TRANS ; - bdev->bd_fsfreeze_count++; -mutex_unlock (&bdev->bd_fsfreeze_mutex); -return error; - } - } - sb->s_frozen =SB_UNFROZEN ; -smp_wmb (); -wake_up (&sb->s_wait_unfrozen); + error =0 ; +if (--bdev->bd_fsfreeze_count >0 ) + goto out_unlock; + +if (!sb) + goto out_unlock; + +BUG_ON (sb->s_bdev != bdev); +down_write (&sb->s_umount); +if (sb->s_flags &MS_RDONLY ) + goto out_deactivate; + +if (sb->s_op->unfreeze_fs) { + error = sb->s_op->unfreeze_fs (sb); +if (error) { +printk (KERN_ERR +"VFS:Filesystem thaw failed\n" ); + sb->s_frozen =SB_FREEZE_TRANS ; + bdev->bd_fsfreeze_count++; +mutex_unlock (&bdev->bd_fsfreeze_mutex); +return error; } -drop_super (sb); } + sb->s_frozen =SB_UNFROZEN ; +smp_wmb (); +wake_up (&sb->s_wait_unfrozen); + +out_deactivate: +if (sb) +deactivate_locked_super (sb); +out_unlock:mutex_unlock (&bdev->bd_fsfreeze_mutex);return 0 ; }
fs/super.c +47 -1
64 lines not on this page · raw message
include/linux/fs.h +1 -0
@@ -2320,6 +2320,7 @@ extern void get_filesystem(struct file_sextern void put_filesystem (struct file_system_type *fs);extern struct file_system_type *get_fs_type (const char *name);extern struct super_block *get_super (struct block_device *); +extern struct super_block *get_active_super (struct block_device *bdev);extern struct super_block *user_get_super (dev_t );extern void drop_super (struct super_block *sb);
PATCH 3/4 Do not allow umounting of frozen filesystems
In reply to Fernando Luis Vázquez CaoInstead 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.
---
fs/namespace.c +12 -0
@@ -1086,6 +1086,14 @@ static int do_umount(struct vfsmount *mnreturn retval; } +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 ; + } + } +down_write (&namespace_sem);spin_lock (&vfsmount_lock); event++;
@@ -1104,6 +1112,10 @@ static int do_umount(struct vfsmount *mnsecurity_sb_umount_busy (mnt);up_write (&namespace_sem);release_mounts (&umount_list); + +if (sb->s_bdev !=NULL ) +mutex_unlock (&sb->s_bdev->bd_fsfreeze_mutex); +return retval; }
Al Viro
maintainerRe: [PATCH 3/4] Do not allow umounting of frozen filesystems
Quoted 8 lines · Fernando Luis V?zquez Cao
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_mountswhich do not return errors.
fs/namespace.c static int do_umount(struct vfsmount *mn · line 1095
Quoted 7 lines
fs/namespace.c +7
-0+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?
Re: [PATCH 3/4] Do not allow umounting of frozen filesystems
Al Viro wrote:
...
Quoted 4 lines
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
Re: [PATCH 3/4] Do not allow umounting of frozen filesystems
Eric Sandeen さんは書きました:
Quoted 9 lines
Al Viro wrote:
...Quoted 3 lines
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.
Quoted 7 lines
Quoted 2 lines
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
Re: [PATCH 3/4] Do not allow umounting of frozen filesystems
In reply to Al ViroAl Viro さんは書きました:
Quoted 25 lines · Fernando Luis V?zquez Cao
Quoted 12 lines
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_mountswhich do not return errors.fs/namespace.c static int do_umount(struct vfsmount *mn · line 1095
Quoted 7 lines
fs/namespace.c +7
-0+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).
Quoted 7 lines
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.
Quoted 3 lines
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
PATCH 4/4 filesystem freeze: add ISFROZEN ioctl
In reply to Fernando Luis Vázquez CaoThe ISFROZEN ioctl can be use by HA and monitoring software to check the freeze state of a filesystem.
---
fs/block_dev.c +15 -0
@@ -334,6 +334,21 @@ out_unlock: }EXPORT_SYMBOL (thaw_bdev); +/** +* +*/ +int isfrozen_bdev (struct block_device *bdev,struct super_block *sb) +{ +int ret; + +mutex_lock (&bdev->bd_fsfreeze_mutex); + ret = sb->s_frozen >SB_UNFROZEN ?1 :0 ; +mutex_unlock (&bdev->bd_fsfreeze_mutex); + +return ret; +} +EXPORT_SYMBOL (isfrozen_bdev); +static int blkdev_writepage (struct page *page,struct writeback_control *wbc) {return block_write_full_page (page, blkdev_get_block, wbc);
fs/ioctl.c +20 -0
@@ -536,6 +536,20 @@ static int ioctl_fsthaw(struct file *filreturn thaw_bdev (sb->s_bdev, sb); } +static int ioctl_isfrozen (struct file *filp) +{ +struct super_block *sb = filp->f_path.dentry->d_inode->i_sb; + +if (!capable (CAP_SYS_ADMIN )) +return -EPERM ; + +/* If a blockdevice-backed filesystem isn't specified, return EINVAL. */ +if (sb->s_bdev ==NULL ) +return -EINVAL ; + +return isfrozen_bdev (sb->s_bdev, sb); +} +/* * When you add any new common ioctls to the switches above and below * please update compat_sys_ioctl() too.
@@ -586,6 +600,12 @@ int do_vfs_ioctl(struct file *filp, unsi error =ioctl_fsthaw (filp);break ; +case FIISFROZEN : + error =ioctl_isfrozen (filp); +if (error >=0 ) +return put_user (error, (int __user *)arg); +break ; +case FS_IOC_FIEMAP :return ioctl_fiemap (filp, arg);
include/linux/fs.h +8 -0
@@ -306,6 +306,7 @@ struct inodes_stat_t {#define FIGETBSZ _IO(0x00,2)/* get the block size used for bmap */ #define FIFREEZE _IOWR('X', 119, int)/* Freeze */ #define FITHAW _IOWR('X', 120, int)/* Thaw */ +#define FIISFROZEN _IOWR('X', 121, int)/* Is frozen? */ #define FS_IOC_GETFLAGS _IOR('f', 1, long)#define FS_IOC_SETFLAGS _IOW('f', 2, long)
@@ -1955,6 +1956,7 @@ extern int sync_blockdev(struct block_deextern struct super_block *freeze_bdev (struct block_device *);extern void emergency_thaw_all (void );extern int thaw_bdev (struct block_device *bdev,struct super_block *sb); +extern int isfrozen_bdev (struct block_device *bdev,struct super_block *sb);extern int fsync_bdev (struct block_device *);#else static inline void bd_forget (struct inode *inode) {}
@@ -1970,6 +1972,12 @@ static inline int thaw_bdev(struct block {return 0 ; } + +extern int isfrozen_bdev (struct block_device *bdev,struct super_block *sb) +{ +return 0 ; +} +#endif extern int sync_filesystem (struct super_block *);extern const struct file_operations def_blk_fops;