On Thu, Jan 10, 2008 at 02:43:23PM -0700, Andreas Dilger wrote:
> On Jan 10, 2008 21:03 +0530, Aneesh Kumar K.V wrote:
> > if (i >= sbi->s_mb_order2_reqs) {
> > - i--;
> > - if ((ac->ac_g_ex.fe_len & (~(1 << i))) == 0)
> > + /*
> > + * This should tell if fe_len is exactly power of 2
> > + */
> > + if ((ac->ac_g_ex.fe_len & (~(1 << (i - 1)))) == 0)
> > ac->ac_2order = i;
>
> While you changed i to (i - 1) in the "if" you didn't change it when
> setting ac_2order... Is that incorrect?
Yes that ac_2order should be i - 1;
Will fix it in the next update.
I see that the patch queue update doesn't have most of the changes I
have placed at http://www.radian.org/~kvaneesh/ext4/jan-10-2008-ver2/
>
> > /*
> > + * Yield the CPU here so that we don't get soft lockup
> > */
> > - schedule_timeout(HZ);
> > + schedule();
> > goto repeat;
> > }
> >
> > @@ -3808,7 +3820,7 @@ repeat:
> > printk(KERN_ERR "uh-oh! used pa while discarding\n");
> > dump_stack();
> > current->state = TASK_UNINTERRUPTIBLE;
> > - schedule();
> > + schedule_timeout(HZ);
> > goto repeat;
>
> Is this change to schedule_timeout() intentional? The earlier code is
> removing the use of schedule_timeout. I could be wrong, as I didn't
> follow this discussion closely, but sometimes changes like this happen
> accidentally and people don't look at the patch itself...
The patch queue had it modified from schedule_timeout to schedule(). I
am moving it back to the original version. If we have set the task state
to TASK_UNINTERRUPTIBLE it should be schedule_timeout. And at these
place we intent to wait uninterrupted for 1 sec. The place where we
wanted to just yield is ext4_mb_discard_group_preallocations.
> > +static unsigned long ext4_get_stripe_size(struct ext4_sb_info *sbi)
> > +{
> > + unsigned long stride = le16_to_cpu(sbi->s_es->s_raid_stride);
> > + unsigned long stripe_width = le32_to_cpu(sbi->s_es->s_raid_stripe_width);
> > +
> > + if (sbi->s_stripe && sbi->s_stripe <= sbi->s_blocks_per_group) {
> > + return sbi->s_stripe;
> > + } else if (stripe_width <= sbi->s_blocks_per_group) {
> > + return stripe_width;
> > + } else if (stride <= sbi->s_blocks_per_group) {
> > + return stride;
> > + }
>
> If you are doing "return XXX" you don't need "else".
>
> > + /*
> > + * set the stripe size. If we have specified it via mount option, then
> > + * use the mount option value. If the value specified at mount time is
> > + * greater than the blocks per group use the super block value.
> > + * Allocator needs it be less than blocks per group.
> > + */
> > + sbi->s_stripe = ext4_get_stripe_size(sbi);
>
> This comment should probably go by ext4_get_stripe_size() definition instead
> of here at the caller.
Will move that to the function definition.
-aneesh
-
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
