Re: [PATCH 1/4] vfs: allow filesystem freeze callers to denote who froze the fs

[Shiyang Ruan <ruansy.fnst@fujitsu.com>]

在 2023/5/7 13:23, Luis Chamberlain 写道:
> On Tue, May 02, 2023 at 08:02:18PM -0700, Darrick J. Wong wrote:
>> diff --git a/fs/super.c b/fs/super.c
>> index 04bc62ab7dfe..01891f9e6d5e 100644
>> --- a/fs/super.c
>> +++ b/fs/super.c
>> @@ -1736,18 +1747,33 @@ int freeze_super(struct super_block *sb)
>>   	up_write(&sb->s_umount);
>>   	return 0;
>>   }
>> +
>> +/*
>> + * freeze_super - lock the filesystem and force it into a consistent state
>> + * @sb: the super to lock
>> + *
>> + * Syncs the super to make sure the filesystem is consistent and calls the fs's
>> + * freeze_fs.  Subsequent calls to this without first thawing the fs will return
>> + * -EBUSY.  See the comment for __freeze_super for more information.
>> + */
>> +int freeze_super(struct super_block *sb)
>> +{
>> +	return __freeze_super(sb, USERSPACE_FREEZE_COOKIE);
>> +}
>>   EXPORT_SYMBOL(freeze_super);
>>   
>> -static int thaw_super_locked(struct super_block *sb)
>> +static int thaw_super_locked(struct super_block *sb, unsigned long cookie)
>>   {
>>   	int error;
>>   
>> -	if (sb->s_writers.frozen != SB_FREEZE_COMPLETE) {
>> +	if (sb->s_writers.frozen != SB_FREEZE_COMPLETE ||
>> +	    sb->s_writers.freeze_cookie != cookie) {
>>   		up_write(&sb->s_umount);
>>   		return -EINVAL;
> 
> We get the same by just having drivers use freeze_super(sb, true) in the
> patches I have, ie, we treat it a user-initiated.
> 
> On freeze() we have:
> 
> int freeze_super(struct super_block *sb, bool usercall)
> {
> 	int ret;
> 	
> 	if(!usercall && sb_is_frozen(sb))
> 		return 0;
> 
> 	if (!sb_is_unfrozen(sb))
> 	return -EBUSY;
> 	...
> }
> 
> On thaw we end up with:
> 
> int thaw_super(struct super_block *sb, bool usercall)
> {
> 	int error;
> 
> 	if (!usercall) {
> 		/*
> 		 * If userspace initiated the freeze don't let the kernel
> 		 *  thaw it on return from a kernel initiated freeze.
> 		 */
> 		 if (sb_is_unfrozen(sb) || sb_is_frozen_by_user(sb))
> 		 	return 0;
> 	}
> 
> 	if (!sb_is_frozen(sb))
> 		return -EINVAL;
> 	...
> }
> 
> As I had it, I had made the drivers and the bdev freeze use the usercall as
> true and so there is no change.
> 
> In case there is a filesystem already frozen then which was initiated by
> the filesystem, for whatever reason, the filesystem the kernel auto-freeze
> will chug on happy with the system freeze, it bails out withour error
> and moves on to the next filesystem to freeze.
> 
> Upon thaw, the kernel auto-thaw will detect that the filesystem was
> frozen by user on sb_is_frozen_by_user() and so will just bail and not
> thaw it.

Hi, Luis

Thanks for the great idea.  I also need this upgraded API for a unbind 
mechanism on pmem device, which is finally called in 
xfs_notify_failure.c where we want to freeze the fs to prevent any other 
new file mappings from being created.  In my case, I think we should 
think it as a kernel-initiated freeze, and hope it won't be thaw by 
others, especially userspace-initiated thaw.

In my understanding of your implementation, if there is a 
userspace-initiated thaw, with @usercall is set true, thaw_super(sb, 
true) will ignore any others' freeze and thaw the fs anyway.  But, 
except in my case, I think the order of userspace-initiated freeze/thaw 
may be messed up due to bugs in the user app, then the kernel-initiated 
freeze state could be accidentally broken...  In my opinion, the kernel 
code is more reliable.  Therefore, kernel-initiated freeze should be 
exclusive at least.


--
Thanks,
Ruan.

> 
> If the mechanism you want to introduce is to allow a filesystem to even
> prevent kernel auto-freeze with -EBUSY it begs the question if that
> shouldn't also prevent suspend. Because it would anyway as you have it
> right now with your patch but it would return -EINVAL. I also ask because of
> the possible issues with the filesystem not going to suspend but the backing
> or other possible related devices going to suspend.
> 
> Since I think the goal is to prevent the kernel auto-freeze due to
> online fsck to complete, then I think you *do* want to prevent full
> system suspend from moving forward. In that case, why not just have
> the filesystem check for that and return -EBUSY on its respective
> filesystem sb->s_op->freeze_fs(sb) callback?
> 
>    Luis