Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1570867 > unrolled thread

remove the cmd_type field from struct request

Back to article view | Back to linux.kernel

Contents

  remove the cmd_type field from struct request Christoph Hellwig <hch@lst.de> - 2017-01-31 17:20 +0100
    [PATCH 01/10] scm_blk: remove unneeded REQ_TYPE_FS check Christoph Hellwig <hch@lst.de> - 2017-01-31 17:30 +0100
    [PATCH 05/10] mspro_block: remove pointless prep_fn Christoph Hellwig <hch@lst.de> - 2017-01-31 17:30 +0100
    [PATCH 07/10] nbd: move request validity checking into nbd_send_cmd Christoph Hellwig <hch@lst.de> - 2017-01-31 17:30 +0100
    [PATCH 06/10] nbd: remove REQ_TYPE_DRV_PRIV leftovers Christoph Hellwig <hch@lst.de> - 2017-01-31 17:30 +0100
    [PATCH 04/10] ms_block: remove pointless prep_fn Christoph Hellwig <hch@lst.de> - 2017-01-31 17:30 +0100
    Re: remove the cmd_type field from struct request Jens Axboe <axboe@kernel.dk> - 2017-01-31 19:10 +0100
      Re: remove the cmd_type field from struct request Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2017-01-31 20:00 +0100
        Re: remove the cmd_type field from struct request Jens Axboe <axboe@kernel.dk> - 2017-01-31 22:20 +0100
      Re: remove the cmd_type field from struct request James Bottomley <James.Bottomley@HansenPartnership.com> - 2017-01-31 20:00 +0100
      Re: remove the cmd_type field from struct request Hannes Reinecke <hare@suse.de> - 2017-02-01 08:00 +0100
        Re: remove the cmd_type field from struct request Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> - 2017-02-01 12:00 +0100
      Re: remove the cmd_type field from struct request Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> - 2017-02-01 11:40 +0100
    Re: remove the cmd_type field from struct request Jens Axboe <axboe@kernel.dk> - 2017-01-31 22:10 +0100
    Re: remove the cmd_type field from struct request "Martin K. Petersen" <martin.petersen@oracle.com> - 2017-02-01 04:00 +0100

#1570867 — remove the cmd_type field from struct request

Hi Jens,

this series gets rid of the cmd_type field in struct request and
instead folds it into the REQ_OP* space.  This reduces the size of
struct request, and leads to a single op namespace that drivers
can easily switch on.  Except for the legacy ide driver which has
a mess of different request types [1] this also nicely cleans up the
code.

Note that the patches are on top of the

    "make SCSI passthrough support optional"

series I sent out on Saturday.  To make life easier I also have a git
tree available here:

	git://git.infradead.org/users/hch/block.git cmd_type

	http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/cmd_type

[1] which were a pain in the ass to untangle and debug during development,
it's really time for it to die..

[toc] | [next] | [standalone]

#1570872 — [PATCH 01/10] scm_blk: remove unneeded REQ_TYPE_FS check

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/s390/block/scm_blk.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/s390/block/scm_blk.c b/drivers/s390/block/scm_blk.c
index 9f16ea6..152de68 100644
--- a/drivers/s390/block/scm_blk.c
+++ b/drivers/s390/block/scm_blk.c
@@ -300,13 +300,6 @@ static void scm_blk_request(struct request_queue *rq)
 	struct request *req;
 
 	while ((req = blk_peek_request(rq))) {
-		if (req->cmd_type != REQ_TYPE_FS) {
-			blk_start_request(req);
-			blk_dump_rq_flags(req, KMSG_COMPONENT " bad request");
-			__blk_end_request_all(req, -EIO);
-			continue;
-		}
-
 		if (!scm_permit_request(bdev, req))
 			goto out;
 
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1570873 — [PATCH 05/10] mspro_block: remove pointless prep_fn

This driver will never see non-fs requests, and doesn't do anything
else in the prep_fn.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/memstick/core/mspro_block.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/drivers/memstick/core/mspro_block.c b/drivers/memstick/core/mspro_block.c
index fa0746d..c00d8a2 100644
--- a/drivers/memstick/core/mspro_block.c
+++ b/drivers/memstick/core/mspro_block.c
@@ -827,18 +827,6 @@ static void mspro_block_start(struct memstick_dev *card)
 	spin_unlock_irqrestore(&msb->q_lock, flags);
 }
 
-static int mspro_block_prepare_req(struct request_queue *q, struct request *req)
-{
-	if (req->cmd_type != REQ_TYPE_FS) {
-		blk_dump_rq_flags(req, "MSPro unsupported request");
-		return BLKPREP_KILL;
-	}
-
-	req->rq_flags |= RQF_DONTPREP;
-
-	return BLKPREP_OK;
-}
-
 static void mspro_block_submit_req(struct request_queue *q)
 {
 	struct memstick_dev *card = q->queuedata;
@@ -1228,7 +1216,6 @@ static int mspro_block_init_disk(struct memstick_dev *card)
 	}
 
 	msb->queue->queuedata = card;
-	blk_queue_prep_rq(msb->queue, mspro_block_prepare_req);
 
 	blk_queue_bounce_limit(msb->queue, limit);
 	blk_queue_max_hw_sectors(msb->queue, MSPRO_BLOCK_MAX_PAGES);
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1570875 — [PATCH 07/10] nbd: move request validity checking into nbd_send_cmd

This is where we do the rest of the request handling, which will
become much simpler soon, too.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/nbd.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index d4c6281..8bce1c7c 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -271,6 +271,9 @@ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index)
 	u32 type;
 	u32 tag = blk_mq_unique_tag(req);
 
+	if (req->cmd_type != REQ_TYPE_FS)
+		return -EIO;
+
 	if (req_op(req) == REQ_OP_DISCARD)
 		type = NBD_CMD_TRIM;
 	else if (req_op(req) == REQ_OP_FLUSH)
@@ -280,6 +283,13 @@ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index)
 	else
 		type = NBD_CMD_READ;
 
+	if (rq_data_dir(req) == WRITE &&
+	    (nbd->flags & NBD_FLAG_READ_ONLY)) {
+		dev_err_ratelimited(disk_to_dev(nbd->disk),
+				    "Write on read-only\n");
+		return -EIO;
+	}
+
 	memset(&request, 0, sizeof(request));
 	request.magic = htonl(NBD_REQUEST_MAGIC);
 	request.type = htonl(type);
@@ -503,17 +513,6 @@ static void nbd_handle_cmd(struct nbd_cmd *cmd, int index)
 		goto error_out;
 	}
 
-	if (req->cmd_type != REQ_TYPE_FS)
-		goto error_out;
-
-	if (req->cmd_type == REQ_TYPE_FS &&
-	    rq_data_dir(req) == WRITE &&
-	    (nbd->flags & NBD_FLAG_READ_ONLY)) {
-		dev_err_ratelimited(disk_to_dev(nbd->disk),
-				    "Write on read-only\n");
-		goto error_out;
-	}
-
 	req->errors = 0;
 
 	nsock = nbd->socks[index];
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1570877 — [PATCH 06/10] nbd: remove REQ_TYPE_DRV_PRIV leftovers

Disconnects don't use block layer requests these days, so all handling
of private requests is dead code.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/nbd.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 9fd06ee..d4c6281 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -193,13 +193,6 @@ static enum blk_eh_timer_return nbd_xmit_timeout(struct request *req,
 	set_bit(NBD_TIMEDOUT, &nbd->runtime_flags);
 	req->errors++;
 
-	/*
-	 * If our disconnect packet times out then we're already holding the
-	 * config_lock and could deadlock here, so just set an error and return,
-	 * we'll handle shutting everything down later.
-	 */
-	if (req->cmd_type == REQ_TYPE_DRV_PRIV)
-		return BLK_EH_HANDLED;
 	mutex_lock(&nbd->config_lock);
 	sock_shutdown(nbd);
 	mutex_unlock(&nbd->config_lock);
@@ -510,8 +503,7 @@ static void nbd_handle_cmd(struct nbd_cmd *cmd, int index)
 		goto error_out;
 	}
 
-	if (req->cmd_type != REQ_TYPE_FS &&
-	    req->cmd_type != REQ_TYPE_DRV_PRIV)
+	if (req->cmd_type != REQ_TYPE_FS)
 		goto error_out;
 
 	if (req->cmd_type == REQ_TYPE_FS &&
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1570878 — [PATCH 04/10] ms_block: remove pointless prep_fn

This driver will never see non-fs requests, and doesn't do anything
else in the prep_fn.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/memstick/core/ms_block.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/memstick/core/ms_block.c b/drivers/memstick/core/ms_block.c
index f3512404..99e651c 100644
--- a/drivers/memstick/core/ms_block.c
+++ b/drivers/memstick/core/ms_block.c
@@ -2000,16 +2000,6 @@ static int msb_bd_getgeo(struct block_device *bdev,
 	return 0;
 }
 
-static int msb_prepare_req(struct request_queue *q, struct request *req)
-{
-	if (req->cmd_type != REQ_TYPE_FS) {
-		blk_dump_rq_flags(req, "MS unsupported request");
-		return BLKPREP_KILL;
-	}
-	req->rq_flags |= RQF_DONTPREP;
-	return BLKPREP_OK;
-}
-
 static void msb_submit_req(struct request_queue *q)
 {
 	struct memstick_dev *card = q->queuedata;
@@ -2132,7 +2122,6 @@ static int msb_init_disk(struct memstick_dev *card)
 	}
 
 	msb->queue->queuedata = card;
-	blk_queue_prep_rq(msb->queue, msb_prepare_req);
 
 	blk_queue_bounce_limit(msb->queue, limit);
 	blk_queue_max_hw_sectors(msb->queue, MS_BLOCK_MAX_PAGES);
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1570957

On 01/31/2017 07:57 AM, Christoph Hellwig wrote:
> [1] which were a pain in the ass to untangle and debug during development,
> it's really time for it to die..

Outside of the patch series in question, how to we expedite the
euthanasia of IDE? What explicit features/support are we missing through
libata that would need to be added, before we can git rm drivers/ide/ ?

-- 
Jens Axboe

[toc] | [prev] | [next] | [standalone]

#1570995

On 01/31/2017 09:51 PM, James Bottomley wrote:

>>> [1] which were a pain in the ass to untangle and debug during
>>> development, it's really time for it to die..
>>
>> Outside of the patch series in question, how to we expedite the
>> euthanasia of IDE? What explicit features/support are we missing
>> through libata that would need to be added, before we can git rm
>> drivers/ide/?
>
> I thought the primary objection was actually embedded in that libata

    Back at MontaVista, even embedded uses switched to libata years ago...
DaveM thinks we still can't prove that libata works everywhere the IDE works.
Besided, there are still a number of non-x86 drivers not converted over to 
libata (like DaVinci, etc.)...

> with its reliance on SCSI was just too large a dependency, so they have
> to keep using drivers/ide.

    Do you remember how many years ago a libata's own block driver was 
promised to you? ;-)
    I'd gladly wrote one, getting tired of my current OSS work... :-)

> Perhaps nvme and flash is obviating this
> problem and we can ask them again, though?

    What's wrong with those? I'm not really following NVMe...

> James

MBR, Sergei

[toc] | [prev] | [next] | [standalone]

#1571099

On 01/31/2017 10:58 AM, Sergei Shtylyov wrote:
> On 01/31/2017 09:51 PM, James Bottomley wrote:
> 
>>>> [1] which were a pain in the ass to untangle and debug during
>>>> development, it's really time for it to die..
>>>
>>> Outside of the patch series in question, how to we expedite the
>>> euthanasia of IDE? What explicit features/support are we missing
>>> through libata that would need to be added, before we can git rm
>>> drivers/ide/?
>>
>> I thought the primary objection was actually embedded in that libata
> 
>     Back at MontaVista, even embedded uses switched to libata years ago...

Indeed, that would be my assumption as well, not too worried about that
side.

> DaveM thinks we still can't prove that libata works everywhere the IDE works.
> Besided, there are still a number of non-x86 drivers not converted over to 
> libata (like DaVinci, etc.)...

That argument is getting harder to buy. IDE has become a considerable
maintenance burden - not for Dave, but for the rest of us that have
to carry a subsystem forward. Personally I would _love_ to kill IDE
at some point in the future, where that future hopefully isn't too far
off. But if we have hardware that is being used and where IDE works and
libata support does not exist, then weneed to fix that first.

>     Do you remember how many years ago a libata's own block driver was 
> promised to you? ;-)
>     I'd gladly wrote one, getting tired of my current OSS work... :-)

Pretty sure I told Jeff originally that libata should only go into the
kernel, if there was a plan to make it independent of SCSI. A promise
was made that of course it would, but that promise was never held,
unfortunately.

-- 
Jens Axboe

[toc] | [prev] | [next] | [standalone]

#1571005

On Tue, 2017-01-31 at 10:02 -0800, Jens Axboe wrote:
> On 01/31/2017 07:57 AM, Christoph Hellwig wrote:
> > [1] which were a pain in the ass to untangle and debug during 
> > development, it's really time for it to die..
> 
> Outside of the patch series in question, how to we expedite the
> euthanasia of IDE? What explicit features/support are we missing 
> through libata that would need to be added, before we can git rm 
> drivers/ide/?

I thought the primary objection was actually embedded in that libata
with its reliance on SCSI was just too large a dependency, so they have
to keep using drivers/ide.  Perhaps nvme and flash is obviating this
problem and we can ask them again, though?

James

[toc] | [prev] | [next] | [standalone]

#1571286

On 01/31/2017 07:02 PM, Jens Axboe wrote:
> On 01/31/2017 07:57 AM, Christoph Hellwig wrote:
>> [1] which were a pain in the ass to untangle and debug during development,
>> it's really time for it to die..
>
> Outside of the patch series in question, how to we expedite the
> euthanasia of IDE? What explicit features/support are we missing through
> libata that would need to be added, before we can git rm drivers/ide/ ?
>
There is only a single driver (sgi_ide) which hasn't been moved over to 
libata. But this is for an old Itanium-based server only, and even SGI 
didn't press us to have this ported.
(And we have disabled the IDE drivers since SLES11, where we still 
support Itanium.)
So they can be safely assumed defunct at this time.

I'm all for removing them.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

[toc] | [prev] | [next] | [standalone]

#1571477

On 2/1/2017 9:53 AM, Hannes Reinecke wrote:

>>> [1] which were a pain in the ass to untangle and debug during development,
>>> it's really time for it to die..
>>
>> Outside of the patch series in question, how to we expedite the
>> euthanasia of IDE? What explicit features/support are we missing through
>> libata that would need to be added, before we can git rm drivers/ide/ ?
>>
> There is only a single driver (sgi_ide) which hasn't been moved over to
> libata.

    Huh? What about Toshiba TC86C001 and TX4938/9, TI DaVinci? That's off the 
top of my head only...

> But this is for an old Itanium-based server only, and even SGI didn't
> press us to have this ported.
> (And we have disabled the IDE drivers since SLES11, where we still support
> Itanium.)

    Or do you mean only the drivers SuSe is interested in?

[...]
> Cheers,
>
> Hannes

MBR, Sergei

[toc] | [prev] | [next] | [standalone]

#1571470

Hi,

On Tuesday, January 31, 2017 10:02:50 AM Jens Axboe wrote:
> On 01/31/2017 07:57 AM, Christoph Hellwig wrote:
> > [1] which were a pain in the ass to untangle and debug during development,
> > it's really time for it to die..
> 
> Outside of the patch series in question, how to we expedite the
> euthanasia of IDE? What explicit features/support are we missing through

When it comes to missing features/support there is still
number of non-x86 host drivers not ported to libata (I'm
slowly working on this as a side-task, any help would be
much appreciated).

> libata that would need to be added, before we can git rm drivers/ide/ ?

I was trying to start the removal with [1] last year but
it has been NAK-ed by DaveM who seems to want to keep
drivers/ide/ forever [2].

[1] https://lkml.org/lkml/2016/2/4/409
[2] https://lkml.org/lkml/2016/12/8/423

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

[toc] | [prev] | [next] | [standalone]

#1571095

On 01/31/2017 07:57 AM, Christoph Hellwig wrote:
> Hi Jens,
> 
> this series gets rid of the cmd_type field in struct request and
> instead folds it into the REQ_OP* space.  This reduces the size of
> struct request, and leads to a single op namespace that drivers
> can easily switch on.  Except for the legacy ide driver which has
> a mess of different request types [1] this also nicely cleans up the
> code.
> 
> Note that the patches are on top of the
> 
>     "make SCSI passthrough support optional"
> 
> series I sent out on Saturday.  To make life easier I also have a git
> tree available here:
> 
> 	git://git.infradead.org/users/hch/block.git cmd_type
> 
> 	http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/cmd_type

Looks good to me, applied for 4.11.

-- 
Jens Axboe

[toc] | [prev] | [next] | [standalone]

#1571253

>>>>> "Christoph" == Christoph Hellwig <hch@lst.de> writes:

Christoph> this series gets rid of the cmd_type field in struct
Christoph> request and instead folds it into the REQ_OP* space.  This
Christoph> reduces the size of struct request, and leads to a single op
Christoph> namespace that drivers can easily switch on.

Very nice cleanup!

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web