linux-next: manual merge of the vfs-brauner tree with the djw-vfs tree

linux-next: manual merge of the vfs-brauner tree with the djw-vfs tree

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 5561 bytes --]

Hi all,

Today's linux-next merge of the vfs-brauner tree got a conflict in:

  fs/super.c

between commits:

  880b9577855e ("fs: distinguish between user initiated freeze and kernel initiated freeze")
  59ba4fdd2d1f ("fs: wait for partially frozen filesystems")

from the djw-vfs tree and commits:

  0ed33598ddf3 ("super: use locking helpers")
  5e8749141521 ("super: wait for nascent superblocks")

from the vfs-brauner tree.

I fixed it up (I think - see below) and can carry the fix as
necessary. This is now fixed as far as linux-next is concerned, but any
non trivial conflicts should be mentioned to your upstream maintainer
when your tree is submitted for merging.  You may also want to consider
cooperating with the maintainer of the conflicting tree to minimise any
particularly complex conflicts.

-- 
Cheers,
Stephen Rothwell

diff --cc fs/super.c
index da68584815e4,a00e9f706f0f..000000000000
--- a/fs/super.c
+++ b/fs/super.c
@@@ -1027,12 -1196,13 +1196,13 @@@ void emergency_remount(void
  
  static void do_thaw_all_callback(struct super_block *sb)
  {
- 	down_write(&sb->s_umount);
- 	if (sb->s_root && sb->s_flags & SB_BORN) {
+ 	bool born = super_lock_excl(sb);
+ 
+ 	if (born && sb->s_root) {
  		emergency_thaw_bdev(sb);
 -		thaw_super_locked(sb);
 +		thaw_super_locked(sb, FREEZE_HOLDER_USERSPACE);
  	} else {
- 		up_write(&sb->s_umount);
+ 		super_unlock_excl(sb);
  	}
  }
  
@@@ -1644,24 -1836,6 +1836,24 @@@ static void sb_freeze_unlock(struct sup
  		percpu_up_write(sb->s_writers.rw_sem + level);
  }
  
 +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);
++		super_unlock_excl(sb);
 +		ret = wait_var_event_killable(&sb->s_writers.frozen,
 +					       sb->s_writers.frozen != old);
- 		down_write(&sb->s_umount);
++		__super_lock_excl(sb);
 +	} while (ret == 0 &&
 +		 sb->s_writers.frozen != SB_UNFROZEN &&
 +		 sb->s_writers.frozen != SB_FREEZE_COMPLETE);
 +
 +	return ret;
 +}
 +
  /**
   * freeze_super - lock the filesystem and force it into a consistent state
   * @sb: the super to lock
@@@ -1711,34 -1874,10 +1903,34 @@@ int freeze_super(struct super_block *sb
  	int ret;
  
  	atomic_inc(&sb->s_active);
- 	down_write(&sb->s_umount);
+ 	__super_lock_excl(sb);
 +
 +retry:
 +	if (sb->s_writers.frozen == SB_FREEZE_COMPLETE) {
 +		if (sb->s_writers.freeze_holders & who) {
 +			deactivate_locked_super(sb);
 +			return -EBUSY;
 +		}
 +
 +		WARN_ON(sb->s_writers.freeze_holders == 0);
 +
 +		/*
 +		 * Someone else already holds this type of freeze; share the
 +		 * freeze and assign the active ref to the freeze.
 +		 */
 +		sb->s_writers.freeze_holders |= who;
- 		up_write(&sb->s_umount);
++		super_unlock_excl(sb);
 +		return 0;
 +	}
 +
  	if (sb->s_writers.frozen != SB_UNFROZEN) {
 -		deactivate_locked_super(sb);
 -		return -EBUSY;
 +		ret = wait_for_partially_frozen(sb);
 +		if (ret) {
 +			deactivate_locked_super(sb);
 +			return ret;
 +		}
 +
 +		goto retry;
  	}
  
  	if (!(sb->s_flags & SB_BORN)) {
@@@ -1748,10 -1887,8 +1940,10 @@@
  
  	if (sb_rdonly(sb)) {
  		/* Nothing to do really... */
 +		sb->s_writers.freeze_holders |= who;
  		sb->s_writers.frozen = SB_FREEZE_COMPLETE;
 +		wake_up_var(&sb->s_writers.frozen);
- 		up_write(&sb->s_umount);
+ 		super_unlock_excl(sb);
  		return 0;
  	}
  
@@@ -1795,11 -1930,9 +1987,11 @@@
  	 * For debugging purposes so that fs can warn if it sees write activity
  	 * when frozen is set to SB_FREEZE_COMPLETE, and for thaw_super().
  	 */
 +	sb->s_writers.freeze_holders |= who;
  	sb->s_writers.frozen = SB_FREEZE_COMPLETE;
 +	wake_up_var(&sb->s_writers.frozen);
  	lockdep_sb_freeze_release(sb);
- 	up_write(&sb->s_umount);
+ 	super_unlock_excl(sb);
  	return 0;
  }
  EXPORT_SYMBOL(freeze_super);
@@@ -1814,24 -1941,8 +2006,24 @@@ static int thaw_super_locked(struct sup
  {
  	int error;
  
 -	if (sb->s_writers.frozen != SB_FREEZE_COMPLETE) {
 +	if (sb->s_writers.frozen == SB_FREEZE_COMPLETE) {
 +		if (!(sb->s_writers.freeze_holders & who)) {
- 			up_write(&sb->s_umount);
++			super_unlock_excl(sb);
 +			return -EINVAL;
 +		}
 +
 +		/*
 +		 * Freeze is shared with someone else.  Release our hold and
 +		 * drop the active ref that freeze_super assigned to the
 +		 * freezer.
 +		 */
 +		if (sb->s_writers.freeze_holders & ~who) {
 +			sb->s_writers.freeze_holders &= ~who;
 +			deactivate_locked_super(sb);
 +			return 0;
 +		}
 +	} else {
- 		up_write(&sb->s_umount);
+ 		super_unlock_excl(sb);
  		return -EINVAL;
  	}
  
@@@ -1867,19 -1974,13 +2059,19 @@@ out
  /**
   * thaw_super -- unlock filesystem
   * @sb: the super to thaw
 + * @who: context that wants to freeze
   *
 - * Unlocks the filesystem and marks it writeable again after freeze_super().
 + * Unlocks the filesystem and marks it writeable again after freeze_super()
 + * if there are no remaining freezes on the filesystem.
 + *
 + * @who should be:
 + * * %FREEZE_HOLDER_USERSPACE if userspace wants to thaw the fs;
 + * * %FREEZE_HOLDER_KERNEL if the kernel wants to thaw the fs.
   */
 -int thaw_super(struct super_block *sb)
 +int thaw_super(struct super_block *sb, enum freeze_holder who)
  {
- 	down_write(&sb->s_umount);
+ 	__super_lock_excl(sb);
 -	return thaw_super_locked(sb);
 +	return thaw_super_locked(sb, who);
  }
  EXPORT_SYMBOL(thaw_super);
  

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: linux-next: manual merge of the vfs-brauner tree with the djw-vfs tree

[Darrick J. Wong]

On Tue, Aug 22, 2023 at 11:05:51AM +1000, Stephen Rothwell wrote:
> Hi all,
> 
> Today's linux-next merge of the vfs-brauner tree got a conflict in:
> 
>   fs/super.c
> 
> between commits:
> 
>   880b9577855e ("fs: distinguish between user initiated freeze and kernel initiated freeze")
>   59ba4fdd2d1f ("fs: wait for partially frozen filesystems")
> 
> from the djw-vfs tree and commits:
> 
>   0ed33598ddf3 ("super: use locking helpers")
>   5e8749141521 ("super: wait for nascent superblocks")
> 
> from the vfs-brauner tree.
> 
> I fixed it up (I think - see below) and can carry the fix as
> necessary. This is now fixed as far as linux-next is concerned, but any
> non trivial conflicts should be mentioned to your upstream maintainer
> when your tree is submitted for merging.  You may also want to consider
> cooperating with the maintainer of the conflicting tree to minimise any
> particularly complex conflicts.

That looks correct, thank you.

Christian: I've been planning to merge the {freeze,thaw}_super @who
changes for 6.6; do you think more 'cooperating with the maintainer' is
needed, or shall I simply push my branch to Linus with a note that
s/down_write/super_lock_excl/ s/up_write/super_unlock_excl is needed to
resolve the merge the conflict?

--D

> -- 
> Cheers,
> Stephen Rothwell
> 
> diff --cc fs/super.c
> index da68584815e4,a00e9f706f0f..000000000000
> --- a/fs/super.c
> +++ b/fs/super.c
> @@@ -1027,12 -1196,13 +1196,13 @@@ void emergency_remount(void
>   
>   static void do_thaw_all_callback(struct super_block *sb)
>   {
> - 	down_write(&sb->s_umount);
> - 	if (sb->s_root && sb->s_flags & SB_BORN) {
> + 	bool born = super_lock_excl(sb);
> + 
> + 	if (born && sb->s_root) {
>   		emergency_thaw_bdev(sb);
>  -		thaw_super_locked(sb);
>  +		thaw_super_locked(sb, FREEZE_HOLDER_USERSPACE);
>   	} else {
> - 		up_write(&sb->s_umount);
> + 		super_unlock_excl(sb);
>   	}
>   }
>   
> @@@ -1644,24 -1836,6 +1836,24 @@@ static void sb_freeze_unlock(struct sup
>   		percpu_up_write(sb->s_writers.rw_sem + level);
>   }
>   
>  +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);
> ++		super_unlock_excl(sb);
>  +		ret = wait_var_event_killable(&sb->s_writers.frozen,
>  +					       sb->s_writers.frozen != old);
> - 		down_write(&sb->s_umount);
> ++		__super_lock_excl(sb);
>  +	} while (ret == 0 &&
>  +		 sb->s_writers.frozen != SB_UNFROZEN &&
>  +		 sb->s_writers.frozen != SB_FREEZE_COMPLETE);
>  +
>  +	return ret;
>  +}
>  +
>   /**
>    * freeze_super - lock the filesystem and force it into a consistent state
>    * @sb: the super to lock
> @@@ -1711,34 -1874,10 +1903,34 @@@ int freeze_super(struct super_block *sb
>   	int ret;
>   
>   	atomic_inc(&sb->s_active);
> - 	down_write(&sb->s_umount);
> + 	__super_lock_excl(sb);
>  +
>  +retry:
>  +	if (sb->s_writers.frozen == SB_FREEZE_COMPLETE) {
>  +		if (sb->s_writers.freeze_holders & who) {
>  +			deactivate_locked_super(sb);
>  +			return -EBUSY;
>  +		}
>  +
>  +		WARN_ON(sb->s_writers.freeze_holders == 0);
>  +
>  +		/*
>  +		 * Someone else already holds this type of freeze; share the
>  +		 * freeze and assign the active ref to the freeze.
>  +		 */
>  +		sb->s_writers.freeze_holders |= who;
> - 		up_write(&sb->s_umount);
> ++		super_unlock_excl(sb);
>  +		return 0;
>  +	}
>  +
>   	if (sb->s_writers.frozen != SB_UNFROZEN) {
>  -		deactivate_locked_super(sb);
>  -		return -EBUSY;
>  +		ret = wait_for_partially_frozen(sb);
>  +		if (ret) {
>  +			deactivate_locked_super(sb);
>  +			return ret;
>  +		}
>  +
>  +		goto retry;
>   	}
>   
>   	if (!(sb->s_flags & SB_BORN)) {
> @@@ -1748,10 -1887,8 +1940,10 @@@
>   
>   	if (sb_rdonly(sb)) {
>   		/* Nothing to do really... */
>  +		sb->s_writers.freeze_holders |= who;
>   		sb->s_writers.frozen = SB_FREEZE_COMPLETE;
>  +		wake_up_var(&sb->s_writers.frozen);
> - 		up_write(&sb->s_umount);
> + 		super_unlock_excl(sb);
>   		return 0;
>   	}
>   
> @@@ -1795,11 -1930,9 +1987,11 @@@
>   	 * For debugging purposes so that fs can warn if it sees write activity
>   	 * when frozen is set to SB_FREEZE_COMPLETE, and for thaw_super().
>   	 */
>  +	sb->s_writers.freeze_holders |= who;
>   	sb->s_writers.frozen = SB_FREEZE_COMPLETE;
>  +	wake_up_var(&sb->s_writers.frozen);
>   	lockdep_sb_freeze_release(sb);
> - 	up_write(&sb->s_umount);
> + 	super_unlock_excl(sb);
>   	return 0;
>   }
>   EXPORT_SYMBOL(freeze_super);
> @@@ -1814,24 -1941,8 +2006,24 @@@ static int thaw_super_locked(struct sup
>   {
>   	int error;
>   
>  -	if (sb->s_writers.frozen != SB_FREEZE_COMPLETE) {
>  +	if (sb->s_writers.frozen == SB_FREEZE_COMPLETE) {
>  +		if (!(sb->s_writers.freeze_holders & who)) {
> - 			up_write(&sb->s_umount);
> ++			super_unlock_excl(sb);
>  +			return -EINVAL;
>  +		}
>  +
>  +		/*
>  +		 * Freeze is shared with someone else.  Release our hold and
>  +		 * drop the active ref that freeze_super assigned to the
>  +		 * freezer.
>  +		 */
>  +		if (sb->s_writers.freeze_holders & ~who) {
>  +			sb->s_writers.freeze_holders &= ~who;
>  +			deactivate_locked_super(sb);
>  +			return 0;
>  +		}
>  +	} else {
> - 		up_write(&sb->s_umount);
> + 		super_unlock_excl(sb);
>   		return -EINVAL;
>   	}
>   
> @@@ -1867,19 -1974,13 +2059,19 @@@ out
>   /**
>    * thaw_super -- unlock filesystem
>    * @sb: the super to thaw
>  + * @who: context that wants to freeze
>    *
>  - * Unlocks the filesystem and marks it writeable again after freeze_super().
>  + * Unlocks the filesystem and marks it writeable again after freeze_super()
>  + * if there are no remaining freezes on the filesystem.
>  + *
>  + * @who should be:
>  + * * %FREEZE_HOLDER_USERSPACE if userspace wants to thaw the fs;
>  + * * %FREEZE_HOLDER_KERNEL if the kernel wants to thaw the fs.
>    */
>  -int thaw_super(struct super_block *sb)
>  +int thaw_super(struct super_block *sb, enum freeze_holder who)
>   {
> - 	down_write(&sb->s_umount);
> + 	__super_lock_excl(sb);
>  -	return thaw_super_locked(sb);
>  +	return thaw_super_locked(sb, who);
>   }
>   EXPORT_SYMBOL(thaw_super);
>   

Re: linux-next: manual merge of the vfs-brauner tree with the djw-vfs tree

[Christian Brauner]

> Christian: I've been planning to merge the {freeze,thaw}_super @who
> changes for 6.6; do you think more 'cooperating with the maintainer' is
> needed, or shall I simply push my branch to Linus with a note that
> s/down_write/super_lock_excl/ s/up_write/super_unlock_excl is needed to
> resolve the merge the conflict?

Hm, that's not a pleasant merge conflict given that it's locking
changes. It would probably be fine to just bring it up the way it is but
it looks needlessly messy/uncoordinated. I'm wonder why this isn't just
all in vfs.super since it's core vfs infra change anyway. Maybe I just
missed the patches if so then sorry about that.

That's the two infrastructure patches in the kernel-fsfreeze
branch/kernel-fsfreeze_2023-07-27 tag?:

ad0164493b81 ("fs: distinguish between user initiated freeze and kernel initiated freeze")
53f65fd7a3d5 ("fs: wait for partially frozen filesystemskernel-fsfreeze_2023-07-27kernel-fsfreeze")

If you give me a tag with your description and just the two commits or I
just cherry pick them and cite your description in my pr that would be
my preferred solution. How do you feel about that?

Re: linux-next: manual merge of the vfs-brauner tree with the djw-vfs tree

[Darrick J. Wong]

On Tue, Aug 22, 2023 at 11:46:38AM +0200, Christian Brauner wrote:
> > Christian: I've been planning to merge the {freeze,thaw}_super @who
> > changes for 6.6; do you think more 'cooperating with the maintainer' is
> > needed, or shall I simply push my branch to Linus with a note that
> > s/down_write/super_lock_excl/ s/up_write/super_unlock_excl is needed to
> > resolve the merge the conflict?
> 
> Hm, that's not a pleasant merge conflict given that it's locking
> changes. It would probably be fine to just bring it up the way it is but
> it looks needlessly messy/uncoordinated. I'm wonder why this isn't just
> all in vfs.super since it's core vfs infra change anyway. Maybe I just
> missed the patches if so then sorry about that.

Ah, I had wondered about that. :)

> That's the two infrastructure patches in the kernel-fsfreeze
> branch/kernel-fsfreeze_2023-07-27 tag?:
> 
> ad0164493b81 ("fs: distinguish between user initiated freeze and kernel initiated freeze")
> 53f65fd7a3d5 ("fs: wait for partially frozen filesystemskernel-fsfreeze_2023-07-27kernel-fsfreeze")
> 
> If you give me a tag with your description and just the two commits or I
> just cherry pick them and cite your description in my pr that would be
> my preferred solution. How do you feel about that?

I'm happy to have you pull my xfs-linux tags into your vfs tree. :)

Here's a tag with just the two vfs patches:
https://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git/tag/?h=vfs-6.6-merge-2

This second tag builds on that, by adding the first actual user of
FREEZE_HOLDER_KERNEL:
https://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git/tag/?h=vfs-6.6-merge-3

There will be more for 6.7(+?) if Luis manages to get back to his
auto-fsfreeze during suspend, or if Shiyang finishes the series to
handle pmem media error reporting in xfs.

Thanks!
--D

Re: linux-next: manual merge of the vfs-brauner tree with the djw-vfs tree

[Christian Brauner]

> > my preferred solution. How do you feel about that?
> 
> I'm happy to have you pull my xfs-linux tags into your vfs tree. :)

Ah, sweet. I apppreciate that. I'll mention in the pr to Linus that if
he wants to reject other parts of the super work that he should then
still simply pull the freeze stuff from you without the rest.

> 
> Here's a tag with just the two vfs patches:
> https://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git/tag/?h=vfs-6.6-merge-2
> 
> This second tag builds on that, by adding the first actual user of
> FREEZE_HOLDER_KERNEL:
> https://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git/tag/?h=vfs-6.6-merge-3

Assuming I understood correctly I did just pull both tags and pushed
them out. Would you please take a look at
https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/log/?h=vfs.super
and let me know if everything looks as expected? I'll be going afk in a
bit just waiting for the kernel build to finish to kick of some
xfstests. If you find anything I'll fix up any issues up tomorrow
morning.

> 
> There will be more for 6.7(+?) if Luis manages to get back to his
> auto-fsfreeze during suspend, or if Shiyang finishes the series to
> handle pmem media error reporting in xfs.

Ok, sounds good let me know/Cc me when ready/needed.

Thanks for all the help!

Re: linux-next: manual merge of the vfs-brauner tree with the djw-vfs tree

[Darrick J. Wong]

On Tue, Aug 22, 2023 at 10:19:00PM +0200, Christian Brauner wrote:
> > > my preferred solution. How do you feel about that?
> > 
> > I'm happy to have you pull my xfs-linux tags into your vfs tree. :)
> 
> Ah, sweet. I apppreciate that. I'll mention in the pr to Linus that if
> he wants to reject other parts of the super work that he should then
> still simply pull the freeze stuff from you without the rest.
> 
> > 
> > Here's a tag with just the two vfs patches:
> > https://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git/tag/?h=vfs-6.6-merge-2
> > 
> > This second tag builds on that, by adding the first actual user of
> > FREEZE_HOLDER_KERNEL:
> > https://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git/tag/?h=vfs-6.6-merge-3
> 
> Assuming I understood correctly I did just pull both tags and pushed
> them out. Would you please take a look at
> https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/log/?h=vfs.super
> and let me know if everything looks as expected? I'll be going afk in a
> bit just waiting for the kernel build to finish to kick of some
> xfstests. If you find anything I'll fix up any issues up tomorrow
> morning.

Hmm.  Looking at the {up,down}_write -> super_{un,}lock_excl conversion,
I think you missed wait_for_partially_frozen:

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;
}

That said, freeze_super() took an s_active refcount at the top, called
super_lock_excl (which means the sb isn't DYING and has been BORN) and
doesn't release it before calling wait_for_partially_frozen.

AFAICT, the subsequent down_write -> super_lock_excl conversions in
freeze_super do not gain us much since I don't think the sb can get to
SB_DYING state without s_active reaching zero, right?  According to
"super: use higher-level helper for {freeze,thaw}", it sounds like the
subsequent down_write calls in freeze_super were replaced for
consistency, even though it "...isn't possible to observe a dying
superblock".

The missing conversion isn't strictly necessary, but it probably makese
sense to do it anyway.

(Aside from that, the conversion looks correct to me.)

> > 
> > There will be more for 6.7(+?) if Luis manages to get back to his
> > auto-fsfreeze during suspend, or if Shiyang finishes the series to
> > handle pmem media error reporting in xfs.
> 
> Ok, sounds good let me know/Cc me when ready/needed.

Will do!

--D

> 
> Thanks for all the help!

Re: linux-next: manual merge of the vfs-brauner tree with the djw-vfs tree

[Christian Brauner]

> Hmm.  Looking at the {up,down}_write -> super_{un,}lock_excl conversion,
> I think you missed wait_for_partially_frozen:

Maha, I sure did. Thanks, converted as well.

> That said, freeze_super() took an s_active refcount at the top, called
> super_lock_excl (which means the sb isn't DYING and has been BORN) and
> doesn't release it before calling wait_for_partially_frozen.

Yes.

> AFAICT, the subsequent down_write -> super_lock_excl conversions in
> freeze_super do not gain us much since I don't think the sb can get to
> SB_DYING state without s_active reaching zero, right?  According to

Yes, if you have an active reference count the superblock stays alive.
If it ever gets into SB_DYING we have a bug.

> The missing conversion isn't strictly necessary, but it probably makese
> sense to do it anyway.

I did. Thanks for pointing that out!

> (Aside from that, the conversion looks correct to me.)

Thanks!