On Wed, Oct 05, 2016 at 11:59:03AM +0200, Jan Kara wrote: > On Wed 05-10-16 08:47:42, Mateusz Guzik wrote: > > On Tue, Oct 04, 2016 at 11:06:15AM +0200, Jan Kara wrote: > > > On Tue 04-10-16 10:53:40, Pierre Morel wrote: > > > > When triggering thaw-filesystems via magic sysrq, the system enters a > > > > loop in do_thaw_one(), as thaw_bdev() still returns success if > > > > bd_fsfreeze_count == 0. To fix this, let thaw_bdev() always return > > > > error (and simplify the code a bit at the same time). > > > > > > > > > > The patch looks good. > > > > > > > Now that I had a closer look, while the patch indeed gets rid of the > > infinite loop, the functionality itself does not work properly. > > > > Note I'm not familiar with this code, so chances are I got details > > wrong. I also don't know the original reasoning. > > > > The current state is that you can freeze by calling either freeze_super > > or freeze_bdev. The latter bumps bd_fsfreeze_count and calls > > freeze_super. freeze_super does NOT modify bd_fsfreeze_count. > > > > freeze_bdev is used by device mapper, xfs and e2fs. > > Where is freeze_bdev() used by e2fs? typo - it's f2fs, and it's usage is copied from XFS. Both of those usages can be completely ignored for th purposes of this argument, because they are done in the context of {X,F2}FS_IOC_FSGOINGDOWN. i.e ioctls to shutdown a filesystem and completely deny access to it. Freezing is just means to an end - it's just used to prevent any more IO from being issued by the block device while we do the filesystem shutdown... IOWs, These are never used in normal usage by users - they are a diagnostic tool and, potentially, a get-out-of-gaol-free card that can be played when something goes wrong with the underlying storage and the filesystem hangs waiting for it. Indeed, the code in both does: sb = freeze_bdev() if (sb && !IS_ERR(sb) { do_shutdown() thaw_bdev(sb) } Which is perfectly sane and will not cause any sort of unbalanced freeze behaviour to occur unless the shutdown oopses, in which case the user has bigger problems than a frozen device... So in production systems, dm_suspend()->lock_fs()->freeze_bdev() is the only path that will ever be exercised. > > Now, looking at *_bdev functions: > > > > > struct super_block *freeze_bdev(struct block_device *bdev) > > > { > > > struct super_block *sb; > > > int error = 0; > > > > > > mutex_lock(&bdev->bd_fsfreeze_mutex); > > > if (++bdev->bd_fsfreeze_count > 1) { > > > > No limit is put in place so in principle this will eventually turn negative. > > Yeah, ok, send a fix... FWIW, that will only overflow if someone tries to freeze their block device more than *2 billion times* without a thaw. If that happens, the user has bigger problems to worry about, like the days/weeks the system couldn't write to the filesystem because it was frozen.... :/ Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx -- 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