Re: ext4-block-reservation.patch

[Andreas Dilger <adilger@clusterfs.com>]

On Jun 19, 2007  22:38 +0530, Aneesh Kumar K.V wrote:
> This is what i have modified. I am yet to build test it. I am looking at 
> forward porting the
> mballoc patches and was planning to send it together.

> +int ext4_reserve_local(struct super_block *sb, int blocks)
> +{
> +	preempt_disable();
> +	rs = sbi->s_reservation_slots + smp_processor_id();

Should this be instead "rs = sbi->s_reservation_slots + get_cpu()"

> +	spin_lock(&rs->rs_lock);
> +	if (likely(rs->rs_reserved >= blocks)) {
> +		rs->rs_reserved -= blocks;
> +		rc = 0;
> +	}
> +	spin_unlock(&rs->rs_lock);
> +
> +	preempt_enable();

And "put_cpu()" here?

> +void ext4_rebalance_reservation(struct ext4_reservation_slot *rs, __u64 free)
> +{
> +	/* chunk is a number of block every used
> +	 * slot will get. make sure it isn't 0 */
> +	chunk = free + used_slots - 1;
> +	do_div(chunk, used_slots);
> +
> +	for_each_possible_cpu(i) {
> +		if (free < chunk)
> +			chunk = free;
> +		if (rs[i].rs_reserved || i == smp_processor_id()) {
> +			rs[i].rs_reserved = chunk;
> +			free -= chunk;
> +			BUG_ON(free < 0);
> +		}
> +	}

Should we be assigning reservations to offline CPUs?  Doesn't it make sense
to assign 0 reservation to offline CPUs until they come back?  In the first
loop, if it is "for_each_possible_cpu()" it would drop reservations from
offline CPUs, and then the bottom one is "for_each_online_cpu()".

Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.