Hi,
In block reservation code while rebalancing the free blocks why are we not
looking at the reservation slots that have no free blocks left. Rebalancing
the free blocks equally across all the reservation slots will make sure
we have less chances of failure later when we try to reserve blocks.
I understand that we consider the CPU slot on which reservation failed while
rebalancing. But what is preventing considering other CPU slot that might have
zero blocks left ?
+void ext4_rebalance_reservation(struct ext4_reservation_slot *rs, __u64 free)
+{
+ int i, used_slots = 0;
+ __u64 chunk;
+
+ /* let's know what slots have been used */
+ for (i = 0; i < NR_CPUS; i++)
+ if (rs[i].rs_reserved || i == smp_processor_id())
+ used_slots++;
+
+ /* 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 (i = 0; i < NR_CPUS; 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);
+ }
+ }
+ BUG_ON(free);
+}
-aneesh
I considered situation when few CPUs get out of blocks at same time rare.
thanks, Alex
Aneesh Kumar K.V wrote:
> Hi,
>
> In block reservation code while rebalancing the free blocks why are we
> not looking at the reservation slots that have no free blocks left.
> Rebalancing
> the free blocks equally across all the reservation slots will make sure
> we have less chances of failure later when we try to reserve blocks.
>
> I understand that we consider the CPU slot on which reservation failed
> while
> rebalancing. But what is preventing considering other CPU slot that
> might have
> zero blocks left ?
>
>
>
>
> +void ext4_rebalance_reservation(struct ext4_reservation_slot *rs, __u64
> free)
> +{
> + int i, used_slots = 0;
> + __u64 chunk;
> +
> + /* let's know what slots have been used */
> + for (i = 0; i < NR_CPUS; i++)
> + if (rs[i].rs_reserved || i == smp_processor_id())
> + used_slots++;
> +
> + /* 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 (i = 0; i < NR_CPUS; 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);
> + }
> + }
> + BUG_ON(free);
> +}
>
>
> -aneesh
> -
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Aneesh Kumar K.V wrote:
> Hi,
>
> In block reservation code while rebalancing the free blocks why are we not
> looking at the reservation slots that have no free blocks left. Rebalancing
> the free blocks equally across all the reservation slots will make sure
> we have less chances of failure later when we try to reserve blocks.
>
>
> I understand that we consider the CPU slot on which reservation failed while
> rebalancing. But what is preventing considering other CPU slot that might have
> zero blocks left ?
>
>
>
>
> +void ext4_rebalance_reservation(struct ext4_reservation_slot *rs, __u64 free)
> +{
> + int i, used_slots = 0;
> + __u64 chunk;
> +
> + /* let's know what slots have been used */
> + for (i = 0; i < NR_CPUS; i++)
BTW... I think you really want:
+ for_each_possible_cpu(i) {
in this and other similar places.
NR_CPUS is a config-time option that may be much more than your actual
count of runtime possible CPUs... on ia64 it's 512 by default, for
example. That's a lot of pointlessness on a 2, 4 or 8 cpu box :)
I can whip up a proper patch for current code to send (again)...
-Eric
> + if (rs[i].rs_reserved || i == smp_processor_id())
> + used_slots++;
> +
> + /* 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 (i = 0; i < NR_CPUS; 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);
> + }
> + }
> + BUG_ON(free);
> +}
>
>
> -aneesh
> -
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Eric Sandeen wrote:
> Aneesh Kumar K.V wrote:
>> Hi,
>>
>> In block reservation code while rebalancing the free blocks why are we not
>> looking at the reservation slots that have no free blocks left. Rebalancing
>> the free blocks equally across all the reservation slots will make sure
>> we have less chances of failure later when we try to reserve blocks.
>>
>>
>> I understand that we consider the CPU slot on which reservation failed while
>> rebalancing. But what is preventing considering other CPU slot that might have
>> zero blocks left ?
>>
>>
>>
>>
>> +void ext4_rebalance_reservation(struct ext4_reservation_slot *rs, __u64 free)
>> +{
>> + int i, used_slots = 0;
>> + __u64 chunk;
>> +
>> + /* let's know what slots have been used */
>> + for (i = 0; i < NR_CPUS; i++)
>
> BTW... I think you really want:
>
> + for_each_possible_cpu(i) {
>
> in this and other similar places.
>
> NR_CPUS is a config-time option that may be much more than your actual
> count of runtime possible CPUs... on ia64 it's 512 by default, for
> example. That's a lot of pointlessness on a 2, 4 or 8 cpu box :)
>
> I can whip up a proper patch for current code to send (again)...
>
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.
-aneesh
diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
index ad3f57c..df6b83c 100644
--- a/fs/ext4/balloc.c
+++ b/fs/ext4/balloc.c
@@ -1981,7 +1981,7 @@ int ext4_reserve_init(struct super_block *sb)
struct ext4_reservation_slot *rs;
int i;
- rs = percpu_alloc(sizeof(struct ext4_reservation_slot), GFP_KERNEL);
+ rs = alloc_percpu(struct ext4_reservation_slot);
if (rs == NULL)
return -ENOMEM;
sbi->s_reservation_slots = rs;
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.