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

From: Joseph Qi <joseph.qi@linux.alibaba.com>
To: Heming Zhao <heming.zhao@suse.com>
Cc: ocfs2-devel@lists.linux.dev, ailiop@suse.com
Subject: Re: [PATCH v4 2/3] ocfs2: adjust enabling place for la window
Date: Thu, 28 Mar 2024 09:58:28 +0800	[thread overview]
Message-ID: <b6038cf4-9966-47ad-b3e5-e0ad76270deb@linux.alibaba.com> (raw)
In-Reply-To: <b7879679-e0e9-43e6-8cc8-1fe3014363d5@suse.com>

On 3/27/24 8:59 PM, Heming Zhao wrote:
> 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;

Seems the above should be along with cancel_delayed_work()?

>>> +        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.
> 

Okay, it will be only set during fill super, so I think it's fine.