From: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
Subject: Re: patch queue update
Date: Fri, 11 Jan 2008 09:39:14 +0530
Message-ID: <20080111040914.GA6959@skywalker>
References: <20080110153358.GA9367@skywalker> <20080110214323.GL3351@webber.adilger.int>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
To: linux-ext4@vger.kernel.org
Content-Disposition: inline
In-Reply-To: <20080110214323.GL3351@webber.adilger.int>
Sender: linux-ext4-owner@vger.kernel.org

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