Re: [PATCH v2 00/13] removing the on-stack struct request

[Jens Axboe <jens.axboe@oracle.com>]

On Mon, May 05 2008, Bartlomiej Zolnierkiewicz wrote:
> On Monday 05 May 2008, FUJITA Tomonori wrote:
> > On Sat, 3 May 2008 15:10:02 +0200
> > Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> wrote:
> > 
> > > 
> > > Hi,
> > > 
> > > On Thursday 01 May 2008, FUJITA Tomonori wrote:
> > > > This is an updated version of the patchset to clean up the asymmetry
> > > > of blk_get/put_request usage:
> > > > 
> > > > http://marc.info/?l=linux-ide&m=120882410712466&w=2
> > > > 
> > > > This patchset removes the code calling blk_put_request against the
> > > > requests that are not allocated via blk_get_request. They can use
> > > > __GFP_WAIT allocation so we can easily convert them to
> > > > use blk_get/put_request properly.
> > > > 
> > > > This patchset enables us to remove the following hack in
> > > > blk_put_request:
> > > > 
> > > > /*
> > > >  * Gee, IDE calls in w/ NULL q.  Fix IDE and remove the
> > > >  * following if (q) test.
> > > >  */
> > > > if (q) {
> > > > 	spin_lock_irqsave(q->queue_lock, flags);
> > > > 	__blk_put_request(q, req);
> > > > 	spin_unlock_irqrestore(q->queue_lock, flags);
> > > > }
> > > > 
> > > > The major changes are:
> > > > 
> > > > - I remove the ide_wait/head_wait path in ide_do_drive_cmd(). It does
> > > > the same thing that blk_execute_rq() does. So let's simply use
> > > > blk_execute_rq(). The nice side effect is that I unexport
> > > > blk_end_sync_rq() since I converted all the users.
> > > > 
> > > > - I drop the unnecessary patch to make ide_do_drive_cmd return
> > > > rq->errors:
> > > > 
> > > > http://marc.info/?l=linux-ide&m=120882410612456&w=2
> > > > 
> > > > #1-10 are for the ide subsystem and #11-13 for the block layer. This
> > > > is against the lastest Linus tree.
> > > 
> > > Thanks.
> > > 
> > > I applied patches #1-10 after making ide_do_drive_cmd() more similar to
> > > blk_execute_rq() (to ease the conversion - it is really better to handle
> > > subtle changes first). [ I'll send these pre-patches in separate mails. ]
> > 
> > Thanks for applying the patches.
> > 
> > 
> > > However I'm still not quite sure about following change in patch #1:
> > > 
> > > @@ -456,24 +452,21 @@ int ide_cdrom_packet(struct cdrom_device_info *cdi,
> > > ...
> > > -	cgc->stat = ide_cd_queue_pc(drive, &req);
> > > +	cgc->stat = ide_cd_queue_pc(drive, cgc->cmd,
> > > +				    cgc->data_direction == CGC_DATA_WRITE,
> > > +				    cgc->buffer, cgc->buflen,
> > > +				    cgc->sense, cgc->timeout, flags);
> > >  	if (!cgc->stat)
> > > -		cgc->buflen -= req.data_len;
> > > +		cgc->buflen = 0;
> > > ...
> > > 
> > > IIRC rq->data_len may be modified while the request is processed?
> > 
> > I think that rq->data_len is modified only when a request is
> > successful but I might overlook or misunderstand something. How about
> > the attached patch (against the latest your quilt patchset)? It's a
> > bit hacky but it should work as before.
> > 
> > 
> > > [ The only other (minor) complaint is that patches #9-10 should be at the
> > >   beginning of the patch series but since no patches contained problems it
> > >   wouldn't be wise to ask respin just for that (and I'm too lazy to review
> > >   it all again ;-). ]
> > 
> > Thanks.
> > 
> > If you are ok with the attached patch, I'm happy to incorporate it
> > into the patchset and send a new version as you like.
> > 
> > 
> > ==
> > From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > Subject: ide: let ide_cd_queue_pc return rq->data_len
> > 
> > ide_cdrom_packet needs rq->data_len after the completion so this patch
> > makes ide_cd_queue_pc return rq->data_len.
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> 
> Thanks!  I just integrated this change with patch #1 so there is no need
> for you to repost the whole patchset.
> 
> WRT patches #11-13: since they are quite straightforward I wonder whether
> it might make sense to push them through IDE tree (they depend on #1-10)?
> 
> Jens, if you are fine with it I'll add your SOB and add them to the queue.

Since they are so small, not an issue with me. You may add my
Signed-off-by, I'm glad that we are (finally) getting rid of the
on-stack requests.

-- 
Jens Axboe