Re: [PATCH v4 2/3] ocfs2: adjust enabling place for la window

[Heming Zhao <heming.zhao@suse.com>]

On 3/27/24 19:05, Joseph Qi wrote:
> 
> 
> On 3/27/24 4:21 PM, Heming Zhao wrote:
>> After introducing gd->bg_contig_free_bits, the code path
>> 'ocfs2_cluster_group_search() => ocfs2_local_alloc_seen_free_bits()'
>> becomes death when all the gd->bg_contig_free_bits are set to the
>> correct value. This patch relocates ocfs2_local_alloc_seen_free_bits()
>> to a more appropriate location. (The new place being
>> ocfs2_block_group_set_bits().)
>>
>> In ocfs2_local_alloc_seen_free_bits(), the scope of the spin-lock has
>> been adjusted to reduce meaningless lock races. e.g: when userspace
>> creates & deletes 1 cluster_size files in parallel, acquiring the
>> spin-lock in ocfs2_local_alloc_seen_free_bits() is totally pointless and
>> impedes IO performance.
>>
>> Signed-off-by: Heming Zhao <heming.zhao@suse.com>
>> ---
>>   fs/ocfs2/localalloc.c | 15 ++++++++-------
>>   fs/ocfs2/suballoc.c   |  9 ++-------
>>   2 files changed, 10 insertions(+), 14 deletions(-)
>>
>> diff --git a/fs/ocfs2/localalloc.c b/fs/ocfs2/localalloc.c
>> index c803c10dd97e..2391b96b8a3b 100644
>> --- a/fs/ocfs2/localalloc.c
>> +++ b/fs/ocfs2/localalloc.c
>> @@ -212,14 +212,15 @@ static inline int ocfs2_la_state_enabled(struct ocfs2_super *osb)
>>   void ocfs2_local_alloc_seen_free_bits(struct ocfs2_super *osb,
>>   				      unsigned int num_clusters)
>>   {
>> -	spin_lock(&osb->osb_lock);
>> -	if (osb->local_alloc_state == OCFS2_LA_DISABLED ||
>> -	    osb->local_alloc_state == OCFS2_LA_THROTTLED)
>> -		if (num_clusters >= osb->local_alloc_default_bits) {
>> +	if (num_clusters >= osb->local_alloc_default_bits) {
>> +		spin_lock(&osb->osb_lock);
>> +		if (osb->local_alloc_state == OCFS2_LA_DISABLED ||
>> +		    osb->local_alloc_state == OCFS2_LA_THROTTLED)
>>   			cancel_delayed_work(&osb->la_enable_wq);
>> -			osb->local_alloc_state = OCFS2_LA_ENABLED;
>> -		}
>> -	spin_unlock(&osb->osb_lock);
>> +
>> +		osb->local_alloc_state = OCFS2_LA_ENABLED;
>> +		spin_unlock(&osb->osb_lock);
>> +	}
> 
> This makes checking osb->local_alloc_default_bits outside osb_lock.
> 
> Joseph

I known, in my view, osb->local_alloc_default_bits doesn't change
after mounting the volume. So, using osb_lock to protect this
variable is pointless.

-Heming