Re: [PATCH 3/7] bdev: implement freeze and thaw holder operations

["Darrick J. Wong" <djwong@kernel.org>]

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!