Re: [PATCH 2/3] fs: wait for partially frozen filesystems

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

On Mon, Jun 12, 2023 at 11:33:02AM -0700, Darrick J. Wong wrote:
> On Sun, Jun 11, 2023 at 09:01:48PM -0700, Christoph Hellwig wrote:
> > On Sun, Jun 11, 2023 at 08:15:28PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > Jan Kara suggested that when one thread is in the middle of freezing a
> > > filesystem, another thread trying to freeze the same fs but with a
> > > different freeze_holder should wait until the freezer reaches either end
> > > state (UNFROZEN or COMPLETE) instead of returning EBUSY immediately.
> > > 
> > > Plumb in the extra coded needed to wait for the fs freezer to reach an
> > > end state and try the freeze again.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > >  fs/super.c |   27 +++++++++++++++++++++++++--
> > >  1 file changed, 25 insertions(+), 2 deletions(-)
> > > 
> > > 
> > > diff --git a/fs/super.c b/fs/super.c
> > > index 36adccecc828..151e0eeff2c2 100644
> > > --- a/fs/super.c
> > > +++ b/fs/super.c
> > > @@ -1647,6 +1647,15 @@ static int freeze_frozen_super(struct super_block *sb, enum freeze_holder who)
> > >  	return 0;
> > >  }
> > >  
> > > +static void wait_for_partially_frozen(struct super_block *sb)
> > > +{
> > > +	up_write(&sb->s_umount);
> > > +	wait_var_event(&sb->s_writers.frozen,
> > > +			sb->s_writers.frozen == SB_UNFROZEN ||
> > > +			sb->s_writers.frozen == SB_FREEZE_COMPLETE);
> > > +	down_write(&sb->s_umount);
> > 
> > Does sb->s_writers.frozen need WRITE_ONCE/READ_ONCE treatment if we want
> > to check it outside of s_umount?  Or should we maybe just open code
> > wait_var_event and only drop the lock after checking the variable?
> 
> How about something like:
> 
> 	do {
> 		up_write(&sb->s_umount);
> 		down_write(&sb->s_umount);
> 	} while (sb->s_writers.frozen != SB_UNFROZEN &&
> 		 sb->s_writers.frozen != SB_FREEZE_COMPLETE);
> 
> so that we always return in either end state of a freezer transition?

Of course as soon as I hit send I realize that no, we don't want to be
cycling s_umount repeatedly even sb->s_writers.frozen hasn't changed.
And maybe we want the wait to be killable too?

static int wait_for_partially_frozen(struct super_block *sb)
{
	int ret = 0;

	do {
		unsigned short old = sb->s_writers.frozen;

		up_write(&sb->s_umount);
		ret = wait_var_event_killable(&sb->s_writers.frozen,
					       sb->s_writers.frozen != old);
		down_write(&sb->s_umount);
	} while (ret == 0 &&
		 sb->s_writers.frozen != SB_UNFROZEN &&
		 sb->s_writers.frozen != SB_FREEZE_COMPLETE);

	return ret;
}

I'll try this out and report back.

--D

> > >  	if (sb->s_writers.frozen != SB_UNFROZEN) {
> > > -		deactivate_locked_super(sb);
> > > -		return -EBUSY;
> > > +		if (!try_again) {
> > > +			deactivate_locked_super(sb);
> > > +			return -EBUSY;
> > > +		}
> > > +
> > > +		wait_for_partially_frozen(sb);
> > > +		try_again = false;
> > > +		goto retry;
> > 
> > Can you throw in a comment on wait we're only waiting for a partial
> > freeze one here?
> 
> I didn't want a thread to get stuck in the retry forever if it always
> loses the race.  However, I think any other threads running freeze_super
> will always end at UNFROZEN or COMPLETE; and thaw_super always goes
> straight froM COMPLETE to UNFROZEN, so I think I'll get rid of the retry
> flag logic entirely.
> 
> --D