LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Martin Schwidefsky <schwidefsky@de.ibm.com>
To: Christoph Hellwig <hch@infradead.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	linux-s390 <linux-s390@vger.kernel.org>,
	Heiko Carstens <heiko.carstens@de.ibm.com>
Subject: Re: [GIT PULL] two more s390 bug fixes for 4.17
Date: Mon, 4 Jun 2018 10:20:22 +0200	[thread overview]
Message-ID: <20180604102022.3bc03229@mschwideX1> (raw)
In-Reply-To: <20180531161400.GB10203@infradead.org>

On Thu, 31 May 2018 09:14:00 -0700
Christoph Hellwig <hch@infradead.org> wrote:

> On Wed, May 30, 2018 at 11:11:40AM +0200, Martin Schwidefsky wrote:
> > On Wed, 30 May 2018 00:41:30 -0700
> > Christoph Hellwig <hch@infradead.org> wrote:
> >   
> > > > -	req->completion_data = cqr;
> > > > +	*((struct dasd_ccw_req **) blk_mq_rq_to_pdu(req)) = cqr;
> > > > +    
> > > 
> > > Please don't play such tricks.  In general your driver structure
> > > should have struct request embedded.  If for some reason
> > > struct dasd_ccw_req has a different life time please create a new
> > > structure instead of these hacks.  
> > 
> > Why do you consider this to be a 'trick'? The blk_mq_rq_to_pdu is
> > meant to be used to access a block of data that is is associated
> > with a request, no? With the change we store a single value, the
> > pointer to a struct dasd_ccw_req. The struct request comes first,
> > later do_dasd_request creates the struct dasd_ccw_req with the
> > CCW chain for the request. And for the blk timeout handler we need
> > a way to find the dasd_ccw_req again.  
> 
> We'll we try really hard to have a structure that we can use
> container_of on.  At least a minimal container with just the pointer
> for a quick fix, but in general it seems like you should be able
> to allocate the whole dasd_ccw_req with the request and just initialize
> it later.

A minimal container with a pointer is easy enough to do. To replace
our allocation scheme for the CCW chains will be hard. For one there
are internal I/Os that do not have an associated 'struct request'.
For those we need to allocate the memory on our own anyway.
Then the size of the memory block you need for the CCW varies wildly
and can be quite large. The tag cmd_size is fixed at the time
blk_mq_alloc_tag_set is called. If we replace out allocation scheme
this would force us to use the maximum size CCW chain for the
tag size, no?

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.

  reply	other threads:[~2018-06-04  8:20 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-30  5:59 Martin Schwidefsky
2018-05-30  7:41 ` Christoph Hellwig
2018-05-30  9:11   ` Martin Schwidefsky
2018-05-31 16:14     ` Christoph Hellwig
2018-06-04  8:20       ` Martin Schwidefsky [this message]
2018-05-30 12:59   ` Sebastian Ott
2018-06-08 17:40     ` Sebastian Ott

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180604102022.3bc03229@mschwideX1 \
    --to=schwidefsky@de.ibm.com \
    --cc=hch@infradead.org \
    --cc=heiko.carstens@de.ibm.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-s390@vger.kernel.org \
    --cc=torvalds@linux-foundation.org \
    --subject='Re: [GIT PULL] two more s390 bug fixes for 4.17' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).