Re: [PATCH 2/2] ext4: mark fc as ineligible using an handle in ext4_xattr_set()

[Luis Henriques <luis.henriques@linux.dev>]

On Thu, Sep 19 2024, Jan Kara wrote:

> On Thu 19-09-24 10:38:48, Luis Henriques (SUSE) wrote:
>> Calling ext4_fc_mark_ineligible() with a NULL handle is racy and may result
>> in a fast-commit being done before the filesystem is effectively marked as
>> ineligible.  This patch reduces the risk of this happening in function
>> ext4_xattr_set() by using an handle if one is available.
>> 
>> Suggested-by: Jan Kara <jack@suse.cz>
>> Signed-off-by: Luis Henriques (SUSE) <luis.henriques@linux.dev>
>
> One comment below:
>
>> diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
>> index 46ce2f21fef9..dbe4d11cd332 100644
>> --- a/fs/ext4/xattr.c
>> +++ b/fs/ext4/xattr.c
>> @@ -2554,11 +2554,15 @@ ext4_xattr_set(struct inode *inode, int name_index, const char *name,
>>  	handle = ext4_journal_start(inode, EXT4_HT_XATTR, credits);
>>  	if (IS_ERR(handle)) {
>>  		error = PTR_ERR(handle);
>> +		ext4_fc_mark_ineligible(inode->i_sb, EXT4_FC_REASON_XATTR,
>> +					NULL);
>
> So when starting a transaction fails:
>
> a) We have a big problem, the journal is aborted so marking fs ineligible
> is moot.
>
> b) We don't set anything and bail with error to userspace so again marking
> fs as ineligible is pointless.
>
> So there's no need to do anything in this case.

Ah! I spent a good amount of time trying to understand if there was a
point marking it as ineligible in that case, but couldn't reach a clear
conclusion.  That's why I decided to leave it there.  And, hoping to get
some early feedback, that's also why I decided to send these 2 patches
first, because fixing ext4_evict_inode() will require a bit more re-work.
The fix will have to deal with more error paths, and probably the call to
ext4_journal_start() will need to be moved upper in the function.

And, as always, thanks a lot your review, Jan.

Cheers,
-- 
Luís

> 								Honza
>
>>  	} else {
>>  		int error2;
>>  
>>  		error = ext4_xattr_set_handle(handle, inode, name_index, name,
>>  					      value, value_len, flags);
>> +		ext4_fc_mark_ineligible(inode->i_sb, EXT4_FC_REASON_XATTR,
>> +					handle);
>>  		error2 = ext4_journal_stop(handle);
>>  		if (error == -ENOSPC &&
>>  		    ext4_should_retry_alloc(sb, &retries))
>> @@ -2566,7 +2570,6 @@ ext4_xattr_set(struct inode *inode, int name_index, const char *name,
>>  		if (error == 0)
>>  			error = error2;
>>  	}
>> -	ext4_fc_mark_ineligible(inode->i_sb, EXT4_FC_REASON_XATTR, NULL);
>>  
>>  	return error;
>>  }
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR