Re: [PATCH 7/7] [BLOCK] Allow elevators to sort/merge discard requests

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

On Fri, Oct 03 2008, Andrew Morton wrote:
> On Sat, 09 Aug 2008 17:33:46 +0100
> David Woodhouse <dwmw2@infradead.org> wrote:
> 
> > Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> > ---
> >  block/blk-core.c       |    2 +-
> >  block/blk-merge.c      |   12 +++++++-----
> >  block/elevator.c       |    4 +++-
> >  include/linux/blkdev.h |    5 +++--
> >  4 files changed, 14 insertions(+), 9 deletions(-)
> 
> I seem to be having a grumpier day.
> 
> ERROR: trailing whitespace
> #96: FILE: block/blk-merge.c:320:
> +^Iif (!bio_has_data(bio) || $
> 
> ERROR: trailing whitespace
> #108: FILE: block/blk-merge.c:360:
> +^Iif (!bio_has_data(bio) || $
> 
> ERROR: trailing whitespace
> #145: FILE: include/linux/blkdev.h:534:
> +#define blk_account_rq(rq)^I(blk_rq_started(rq) && (blk_fs_request(rq) || blk_discard_rq(rq))) $
> 
> WARNING: line over 80 characters
> #145: FILE: include/linux/blkdev.h:534:
> +#define blk_account_rq(rq)     (blk_rq_started(rq) && (blk_fs_request(rq) || blk_discard_rq(rq))) 
> 
> total: 3 errors, 1 warnings, 71 lines checked
> 
> 
> Do we think there were tangible benefits to adding new trailing
> whitespace?

I fixed that up in a later patch.

> > -#define blk_account_rq(rq)	(blk_rq_started(rq) && blk_fs_request(rq))
> > +#define blk_account_rq(rq)	(blk_rq_started(rq) && (blk_fs_request(rq) || blk_discard_rq(rq))) 
> >  
> >  #define blk_pm_suspend_request(rq)	((rq)->cmd_type == REQ_TYPE_PM_SUSPEND)
> >  #define blk_pm_resume_request(rq)	((rq)->cmd_type == REQ_TYPE_PM_RESUME)
> > @@ -588,7 +588,8 @@ static inline void blk_clear_queue_full(struct request_queue *q, int rw)
> >  #define RQ_NOMERGE_FLAGS	\
> >  	(REQ_NOMERGE | REQ_STARTED | REQ_HARDBARRIER | REQ_SOFTBARRIER)
> >  #define rq_mergeable(rq)	\
> > -	(!((rq)->cmd_flags & RQ_NOMERGE_FLAGS) && blk_fs_request((rq)))
> > +	(!((rq)->cmd_flags & RQ_NOMERGE_FLAGS) && \
> > +	 (blk_discard_rq(rq) || blk_fs_request((rq))))
> 
> Can we please stop writing the kernel in cpp?
> 
> Not only are these things completely foul and stupid from a readbility
> POV, they are flat out buggy if passed expression-with-side-effects and
> will generate large amounts of additional text if called with _any_
> expression other than a local variable or something which by some
> miracle GCC can 100% CSE.

It's not real pretty, but I'd be hard pressed to think of a code example
that has ever passed 'rq' as an expression with side effects...

-- 
Jens Axboe