On Wed, Sep 27, 2023 at 05:15:28PM +0200, Christian Brauner wrote: > > > + sync_blockdev(bdev); > > > > Why ignore the return value from sync_blockdev? It calls > > filemap_write_and_wait, which clears AS_EIO/AS_ENOSPC from the bdev > > mapping, which means that we'll silently drop accumulated IO errors. > > Because freeze_bdev() has always ignored sync_blockdev() errors so far > and I'm not sure what we'd do with that error. We can report it but we > might confuse callers that think that the freeze failed when it hasn't. Thinking about this some more... I got confused that fs_bdev_freeze drops the bd_fsfreeze_count if freeze_super fails. But I guess that has to get done before letting go of bd_holder_lock. For the bdev->bd_holder_ops == fs_holder_ops case, the freeze_super call will call sync_filesystem, which calls sync_blockdev. If that fails, the fsfreeze aborts, and the bdev freeze (at least with the old code) would also abort. For the !bdev->bd_holder_ops case, why not capture the sync_blockdev error code and decrement bd_fsfreeze_count if the sync failed? Then this function either returns 0 with the fs and bdev frozen; or an error code and nothing frozen. (Also, does this mean that the new sync_blockdev call at the bottom of fs_bdev_freeze isn't necessary? Filesystems that do IO in ->freeze_fs should be flushing the block device.) --D > > > > > + mutex_unlock(&bdev->bd_holder_lock); > > > > Also not sure why this fallback case holds bd_holder_lock across the > > sync_blockdev but fs_bdev_freeze doesn't? > > I'll massage that to be consistent. Thanks!
