Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 8 Sep 2022 10:57:17 +0200
From:   Jan Kara <jack@suse.cz>
To:     "Ritesh Harjani (IBM)" <ritesh.list@gmail.com>
Cc:     Jan Kara <jack@suse.cz>, Ted Tso <tytso@mit.edu>,
        linux-ext4@vger.kernel.org,
        Thorsten Leemhuis <regressions@leemhuis.info>,
        Ojaswin Mujoo <ojaswin@linux.ibm.com>,
        Stefan Wahren <stefan.wahren@i2se.com>,
        Andreas Dilger <adilger.kernel@dilger.ca>
Subject: Re: [PATCH 2/5] ext4: Avoid unnecessary spreading of allocations
 among groups
Message-ID: <20220908085717.2kln432koqxbz3ja@quack3>
References: <20220906150803.375-1-jack@suse.cz>
 <20220906152920.25584-2-jack@suse.cz>
 <20220907180507.3byq5uts42e6dpkn@riteshh-domain>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220907180507.3byq5uts42e6dpkn@riteshh-domain>
Precedence: bulk

On Wed 07-09-22 23:35:07, Ritesh Harjani (IBM) wrote:
> On 22/09/06 05:29PM, Jan Kara wrote:
> > mb_set_largest_free_order() updates lists containing groups with largest
> > chunk of free space of given order. The way it updates it leads to
> > always moving the group to the tail of the list. Thus allocations
> > looking for free space of given order effectively end up cycling through
> > all groups (and due to initialization in last to first order). This
> > spreads allocations among block groups which reduces performance for
> > rotating disks or low-end flash media. Change
> > mb_set_largest_free_order() to only update lists if the order of the
> > largest free chunk in the group changed.
> 
> Nice and clear explaination. Thanks :)
> 
> This change also looks good to me.
> Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

Thanks for review!

> One other thought to further optimize - 
> Will it make a difference if rather then adding the group to the tail of the list, 
> we add that group to the head of sbi->s_mb_largest_free_orders[new_order]. 
> 
> This is because this group is the latest from where blocks were allocated/freed,
> and hence the next allocation should first try from this group in order to keep 
> the files/extents blocks close to each other? 
> (That sometimes might help with disk firmware to avoid doing discards if the freed 
> block can be reused?)
> 
> Or does goal block will always cover that case by default and we might never
> require this? Maybe in a case of a new file within the same directory where 
> the goal group has no free blocks, but the last group attempted should be 
> retried first?

So I was also wondering about this somewhat. I think that goal group will
take care of keeping file data together so head/tail insertion should not
matter too much for one file. Maybe if the allocation comes from a
different inode, then the head/tail insertion matters but then it is not
certain whether the allocation is actually related and what its order is
(depending on that we might prefer same / different group) so I've decided
to just keep things as they are. I agree it might be interesting to
investigate and experiment with various workloads and see whether the
head/tail insertion makes a difference for some workload but I think it's a
separate project.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR