Re: [PATCH scsi-misc-2.6 08/13] scsi: move request preps in other places into prep_fn()
On Tue, 2005-04-05 at 15:19 +0900, Tejun Heo wrote: > No problem. Do you want me to do that now? Or is it okay to do the > next take after you review the request_fn rewrite patch? Just on resubmit ... I think you're currently reworking the request_fn patch based on Christoph's comments, so resend the sequence when you have this. Thanks, James - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH scsi-misc-2.6 08/13] scsi: move request preps in other places into prep_fn()
Hello, James. James Bottomley wrote: On Fri, 2005-04-01 at 14:25 +0900, Tejun Heo wrote: Ah.. with later requeue path consolidation patches, all requests get their sense buffer cleared during requeueing, which, IMHO, is more logical. Moving scsi_init_cmd_errh() should come after the patch. Sorry. :-) I'll make another take of this patchset (maybe subset) after issues are resolved. I'll split and reorder relocation of scsi_init_cmd_errh then. Thanks. It would help me enormously if you explained what bugs you were fixing at the top of each patch, Well, I'll try harder. > and also only do patchsets that are dependent on each other (I already have your serial_numer_at_timeout and internal_timeout removal patches in the scsi-misc-2.6 tree). No problem. Do you want me to do that now? Or is it okay to do the next take after you review the request_fn rewrite patch? Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH scsi-misc-2.6 08/13] scsi: move request preps in other places into prep_fn()
Hello, James. James Bottomley wrote: On Fri, 2005-04-01 at 14:25 +0900, Tejun Heo wrote: Ah.. with later requeue path consolidation patches, all requests get their sense buffer cleared during requeueing, which, IMHO, is more logical. Moving scsi_init_cmd_errh() should come after the patch. Sorry. :-) I'll make another take of this patchset (maybe subset) after issues are resolved. I'll split and reorder relocation of scsi_init_cmd_errh then. Thanks. It would help me enormously if you explained what bugs you were fixing at the top of each patch, Well, I'll try harder. and also only do patchsets that are dependent on each other (I already have your serial_numer_at_timeout and internal_timeout removal patches in the scsi-misc-2.6 tree). No problem. Do you want me to do that now? Or is it okay to do the next take after you review the request_fn rewrite patch? Thanks. -- tejun - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH scsi-misc-2.6 08/13] scsi: move request preps in other places into prep_fn()
On Tue, 2005-04-05 at 15:19 +0900, Tejun Heo wrote: No problem. Do you want me to do that now? Or is it okay to do the next take after you review the request_fn rewrite patch? Just on resubmit ... I think you're currently reworking the request_fn patch based on Christoph's comments, so resend the sequence when you have this. Thanks, James - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH scsi-misc-2.6 08/13] scsi: move request preps in other places into prep_fn()
On Fri, 2005-04-01 at 14:25 +0900, Tejun Heo wrote: > Ah.. with later requeue path consolidation patches, all requests get > their sense buffer cleared during requeueing, which, IMHO, is more > logical. Moving scsi_init_cmd_errh() should come after the patch. > Sorry. :-) > > I'll make another take of this patchset (maybe subset) after issues > are resolved. I'll split and reorder relocation of scsi_init_cmd_errh > then. Thanks. It would help me enormously if you explained what bugs you were fixing at the top of each patch, and also only do patchsets that are dependent on each other (I already have your serial_numer_at_timeout and internal_timeout removal patches in the scsi-misc-2.6 tree). James - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH scsi-misc-2.6 08/13] scsi: move request preps in other places into prep_fn()
On Fri, 2005-04-01 at 14:25 +0900, Tejun Heo wrote: Ah.. with later requeue path consolidation patches, all requests get their sense buffer cleared during requeueing, which, IMHO, is more logical. Moving scsi_init_cmd_errh() should come after the patch. Sorry. :-) I'll make another take of this patchset (maybe subset) after issues are resolved. I'll split and reorder relocation of scsi_init_cmd_errh then. Thanks. It would help me enormously if you explained what bugs you were fixing at the top of each patch, and also only do patchsets that are dependent on each other (I already have your serial_numer_at_timeout and internal_timeout removal patches in the scsi-misc-2.6 tree). James - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH scsi-misc-2.6 08/13] scsi: move request preps in other places into prep_fn()
Hello, Christoph. On Thu, Mar 31, 2005 at 11:20:40AM +0100, Christoph Hellwig wrote: > > +/* > > + * Macro to determine the size of SCSI command. This macro takes vendor > > + * unique commands into account. SCSI commands in groups 6 and 7 are > > + * vendor unique and we will depend upon the command length being > > + * supplied correctly in cmd_len. > > + */ > > +#define CDB_SIZE(cmd) (cmd)->cmnd[0] >> 5) & 7) < 6) ? \ > > + COMMAND_SIZE((cmd)->cmnd[0]) : (cmd)->cmd_len) > > should probably go to scsi.h as it's generally usefull. I don't know. Currently it's used only in one place. Actually, I was thinking about moving it into the function where it's used. But if it's useful, renaming it to something like SCSI_CMD_CDB_SIZE() (maybe make it inline function?) and moving to scsi.h shouldn't be any problem. I think we need to hear other people's opinions. Some inputs please. Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH scsi-misc-2.6 08/13] scsi: move request preps in other places into prep_fn()
Hello, James. On Thu, Mar 31, 2005 at 12:07:44PM -0600, James Bottomley wrote: > On Thu, 2005-03-31 at 18:08 +0900, Tejun Heo wrote: > > Move request preparations scattered in scsi_request_fn() and > > scsi_dispatch_cmd() into scsi_prep_fn(). > > > > * CDB_SIZE check in scsi_dispatch_cmd() > > * SCSI-2 LUN preparation in scsi_dispatch_cmd() > > * scsi_init_cmd_errh() in scsi_request_fn() > > > > No invalid request reaches scsi_request_fn() anymore. > > This one, I like, there's just one small problem: > > You can't move scsi_init_cmd_errh() out of the request function path: > It's where we set up the sense buffer handling, so it has to be done > every time the command is prepared for execution (the prep function is > only called once)---think what happens if we turn a command around for > retry based on a sense indication. > > So redo the patch and I'll put it in. Ah.. with later requeue path consolidation patches, all requests get their sense buffer cleared during requeueing, which, IMHO, is more logical. Moving scsi_init_cmd_errh() should come after the patch. Sorry. :-) I'll make another take of this patchset (maybe subset) after issues are resolved. I'll split and reorder relocation of scsi_init_cmd_errh then. Thanks. -- tejun - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH scsi-misc-2.6 08/13] scsi: move request preps in other places into prep_fn()
On Thu, 2005-03-31 at 18:08 +0900, Tejun Heo wrote: > Move request preparations scattered in scsi_request_fn() and > scsi_dispatch_cmd() into scsi_prep_fn(). > > * CDB_SIZE check in scsi_dispatch_cmd() > * SCSI-2 LUN preparation in scsi_dispatch_cmd() > * scsi_init_cmd_errh() in scsi_request_fn() > > No invalid request reaches scsi_request_fn() anymore. This one, I like, there's just one small problem: You can't move scsi_init_cmd_errh() out of the request function path: It's where we set up the sense buffer handling, so it has to be done every time the command is prepared for execution (the prep function is only called once)---think what happens if we turn a command around for retry based on a sense indication. So redo the patch and I'll put it in. James - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH scsi-misc-2.6 08/13] scsi: move request preps in other places into prep_fn()
> +/* > + * Macro to determine the size of SCSI command. This macro takes vendor > + * unique commands into account. SCSI commands in groups 6 and 7 are > + * vendor unique and we will depend upon the command length being > + * supplied correctly in cmd_len. > + */ > +#define CDB_SIZE(cmd)(cmd)->cmnd[0] >> 5) & 7) < 6) ? \ > + COMMAND_SIZE((cmd)->cmnd[0]) : (cmd)->cmd_len) should probably go to scsi.h as it's generally usefull. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH scsi-misc-2.6 08/13] scsi: move request preps in other places into prep_fn()
08_scsi_move_preps_to_prep_fn.patch Move request preparations scattered in scsi_request_fn() and scsi_dispatch_cmd() into scsi_prep_fn(). * CDB_SIZE check in scsi_dispatch_cmd() * SCSI-2 LUN preparation in scsi_dispatch_cmd() * scsi_init_cmd_errh() in scsi_request_fn() No invalid request reaches scsi_request_fn() anymore. Signed-off-by: Tejun Heo <[EMAIL PROTECTED]> scsi.c | 30 -- scsi_lib.c | 25 +++-- 2 files changed, 19 insertions(+), 36 deletions(-) Index: scsi-export/drivers/scsi/scsi.c === --- scsi-export.orig/drivers/scsi/scsi.c2005-03-31 18:06:21.0 +0900 +++ scsi-export/drivers/scsi/scsi.c 2005-03-31 18:06:22.0 +0900 @@ -79,15 +79,6 @@ #define MIN_RESET_PERIOD (15*HZ) /* - * Macro to determine the size of SCSI command. This macro takes vendor - * unique commands into account. SCSI commands in groups 6 and 7 are - * vendor unique and we will depend upon the command length being - * supplied correctly in cmd_len. - */ -#define CDB_SIZE(cmd) (cmd)->cmnd[0] >> 5) & 7) < 6) ? \ - COMMAND_SIZE((cmd)->cmnd[0]) : (cmd)->cmd_len) - -/* * Note - the initial logging level can be set here to log events at boot time. * After the system is up, you may enable logging via the /proc interface. */ @@ -566,14 +557,6 @@ int scsi_dispatch_cmd(struct scsi_cmnd * goto out; } - /* -* If SCSI-2 or lower, store the LUN value in cmnd. -*/ - if (cmd->device->scsi_level <= SCSI_2) { - cmd->cmnd[1] = (cmd->cmnd[1] & 0x1f) | - (cmd->device->lun << 5 & 0xe0); - } - /* * We will wait MIN_RESET_DELAY clock ticks after the last reset so * we can avoid the drive not being ready. @@ -614,19 +597,6 @@ int scsi_dispatch_cmd(struct scsi_cmnd * atomic_inc(>device->iorequest_cnt); - /* -* Before we queue this command, check if the command -* length exceeds what the host adapter can handle. -*/ - if (CDB_SIZE(cmd) > cmd->device->host->max_cmd_len) { - SCSI_LOG_MLQUEUE(3, - printk("queuecommand : command too long.\n")); - cmd->result = (DID_ABORT << 16); - - scsi_done(cmd); - goto out; - } - spin_lock_irqsave(host->host_lock, flags); scsi_cmd_get_serial(host, cmd); Index: scsi-export/drivers/scsi/scsi_lib.c === --- scsi-export.orig/drivers/scsi/scsi_lib.c2005-03-31 18:06:21.0 +0900 +++ scsi-export/drivers/scsi/scsi_lib.c 2005-03-31 18:06:22.0 +0900 @@ -28,6 +28,14 @@ #include "scsi_priv.h" #include "scsi_logging.h" +/* + * Macro to determine the size of SCSI command. This macro takes vendor + * unique commands into account. SCSI commands in groups 6 and 7 are + * vendor unique and we will depend upon the command length being + * supplied correctly in cmd_len. + */ +#define CDB_SIZE(cmd) (cmd)->cmnd[0] >> 5) & 7) < 6) ? \ + COMMAND_SIZE((cmd)->cmnd[0]) : (cmd)->cmd_len) #define SG_MEMPOOL_NR (sizeof(scsi_sg_pools)/sizeof(struct scsi_host_sg_pool)) #define SG_MEMPOOL_SIZE32 @@ -1140,6 +1148,17 @@ static int scsi_prep_fn(struct request_q goto kill; } + /* Check command length. */ + if (CDB_SIZE(cmd) > sdev->host->max_cmd_len) + goto kill; + + scsi_init_cmd_errh(cmd); + + /* If SCSI-2 or lower, store the LUN value in cmnd. */ + if (cmd->device->scsi_level <= SCSI_2) + cmd->cmnd[1] = (cmd->cmnd[1] & 0x1f) | + (cmd->device->lun << 5 & 0xe0); + /* * The request is now prepped, no need to come back here */ @@ -1316,12 +1335,6 @@ static void scsi_request_fn(struct reque } /* -* Finally, initialize any error handling parameters, and set up -* the timers for timeouts. -*/ - scsi_init_cmd_errh(cmd); - - /* * Dispatch the command to the low-level driver. */ rtn = scsi_dispatch_cmd(cmd); - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH scsi-misc-2.6 08/13] scsi: move request preps in other places into prep_fn()
08_scsi_move_preps_to_prep_fn.patch Move request preparations scattered in scsi_request_fn() and scsi_dispatch_cmd() into scsi_prep_fn(). * CDB_SIZE check in scsi_dispatch_cmd() * SCSI-2 LUN preparation in scsi_dispatch_cmd() * scsi_init_cmd_errh() in scsi_request_fn() No invalid request reaches scsi_request_fn() anymore. Signed-off-by: Tejun Heo [EMAIL PROTECTED] scsi.c | 30 -- scsi_lib.c | 25 +++-- 2 files changed, 19 insertions(+), 36 deletions(-) Index: scsi-export/drivers/scsi/scsi.c === --- scsi-export.orig/drivers/scsi/scsi.c2005-03-31 18:06:21.0 +0900 +++ scsi-export/drivers/scsi/scsi.c 2005-03-31 18:06:22.0 +0900 @@ -79,15 +79,6 @@ #define MIN_RESET_PERIOD (15*HZ) /* - * Macro to determine the size of SCSI command. This macro takes vendor - * unique commands into account. SCSI commands in groups 6 and 7 are - * vendor unique and we will depend upon the command length being - * supplied correctly in cmd_len. - */ -#define CDB_SIZE(cmd) (cmd)-cmnd[0] 5) 7) 6) ? \ - COMMAND_SIZE((cmd)-cmnd[0]) : (cmd)-cmd_len) - -/* * Note - the initial logging level can be set here to log events at boot time. * After the system is up, you may enable logging via the /proc interface. */ @@ -566,14 +557,6 @@ int scsi_dispatch_cmd(struct scsi_cmnd * goto out; } - /* -* If SCSI-2 or lower, store the LUN value in cmnd. -*/ - if (cmd-device-scsi_level = SCSI_2) { - cmd-cmnd[1] = (cmd-cmnd[1] 0x1f) | - (cmd-device-lun 5 0xe0); - } - /* * We will wait MIN_RESET_DELAY clock ticks after the last reset so * we can avoid the drive not being ready. @@ -614,19 +597,6 @@ int scsi_dispatch_cmd(struct scsi_cmnd * atomic_inc(cmd-device-iorequest_cnt); - /* -* Before we queue this command, check if the command -* length exceeds what the host adapter can handle. -*/ - if (CDB_SIZE(cmd) cmd-device-host-max_cmd_len) { - SCSI_LOG_MLQUEUE(3, - printk(queuecommand : command too long.\n)); - cmd-result = (DID_ABORT 16); - - scsi_done(cmd); - goto out; - } - spin_lock_irqsave(host-host_lock, flags); scsi_cmd_get_serial(host, cmd); Index: scsi-export/drivers/scsi/scsi_lib.c === --- scsi-export.orig/drivers/scsi/scsi_lib.c2005-03-31 18:06:21.0 +0900 +++ scsi-export/drivers/scsi/scsi_lib.c 2005-03-31 18:06:22.0 +0900 @@ -28,6 +28,14 @@ #include scsi_priv.h #include scsi_logging.h +/* + * Macro to determine the size of SCSI command. This macro takes vendor + * unique commands into account. SCSI commands in groups 6 and 7 are + * vendor unique and we will depend upon the command length being + * supplied correctly in cmd_len. + */ +#define CDB_SIZE(cmd) (cmd)-cmnd[0] 5) 7) 6) ? \ + COMMAND_SIZE((cmd)-cmnd[0]) : (cmd)-cmd_len) #define SG_MEMPOOL_NR (sizeof(scsi_sg_pools)/sizeof(struct scsi_host_sg_pool)) #define SG_MEMPOOL_SIZE32 @@ -1140,6 +1148,17 @@ static int scsi_prep_fn(struct request_q goto kill; } + /* Check command length. */ + if (CDB_SIZE(cmd) sdev-host-max_cmd_len) + goto kill; + + scsi_init_cmd_errh(cmd); + + /* If SCSI-2 or lower, store the LUN value in cmnd. */ + if (cmd-device-scsi_level = SCSI_2) + cmd-cmnd[1] = (cmd-cmnd[1] 0x1f) | + (cmd-device-lun 5 0xe0); + /* * The request is now prepped, no need to come back here */ @@ -1316,12 +1335,6 @@ static void scsi_request_fn(struct reque } /* -* Finally, initialize any error handling parameters, and set up -* the timers for timeouts. -*/ - scsi_init_cmd_errh(cmd); - - /* * Dispatch the command to the low-level driver. */ rtn = scsi_dispatch_cmd(cmd); - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH scsi-misc-2.6 08/13] scsi: move request preps in other places into prep_fn()
+/* + * Macro to determine the size of SCSI command. This macro takes vendor + * unique commands into account. SCSI commands in groups 6 and 7 are + * vendor unique and we will depend upon the command length being + * supplied correctly in cmd_len. + */ +#define CDB_SIZE(cmd)(cmd)-cmnd[0] 5) 7) 6) ? \ + COMMAND_SIZE((cmd)-cmnd[0]) : (cmd)-cmd_len) should probably go to scsi.h as it's generally usefull. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH scsi-misc-2.6 08/13] scsi: move request preps in other places into prep_fn()
On Thu, 2005-03-31 at 18:08 +0900, Tejun Heo wrote: Move request preparations scattered in scsi_request_fn() and scsi_dispatch_cmd() into scsi_prep_fn(). * CDB_SIZE check in scsi_dispatch_cmd() * SCSI-2 LUN preparation in scsi_dispatch_cmd() * scsi_init_cmd_errh() in scsi_request_fn() No invalid request reaches scsi_request_fn() anymore. This one, I like, there's just one small problem: You can't move scsi_init_cmd_errh() out of the request function path: It's where we set up the sense buffer handling, so it has to be done every time the command is prepared for execution (the prep function is only called once)---think what happens if we turn a command around for retry based on a sense indication. So redo the patch and I'll put it in. James - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH scsi-misc-2.6 08/13] scsi: move request preps in other places into prep_fn()
Hello, Christoph. On Thu, Mar 31, 2005 at 11:20:40AM +0100, Christoph Hellwig wrote: +/* + * Macro to determine the size of SCSI command. This macro takes vendor + * unique commands into account. SCSI commands in groups 6 and 7 are + * vendor unique and we will depend upon the command length being + * supplied correctly in cmd_len. + */ +#define CDB_SIZE(cmd) (cmd)-cmnd[0] 5) 7) 6) ? \ + COMMAND_SIZE((cmd)-cmnd[0]) : (cmd)-cmd_len) should probably go to scsi.h as it's generally usefull. I don't know. Currently it's used only in one place. Actually, I was thinking about moving it into the function where it's used. But if it's useful, renaming it to something like SCSI_CMD_CDB_SIZE() (maybe make it inline function?) and moving to scsi.h shouldn't be any problem. I think we need to hear other people's opinions. Some inputs please. Thanks. -- tejun - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH scsi-misc-2.6 08/13] scsi: move request preps in other places into prep_fn()
Hello, James. On Thu, Mar 31, 2005 at 12:07:44PM -0600, James Bottomley wrote: On Thu, 2005-03-31 at 18:08 +0900, Tejun Heo wrote: Move request preparations scattered in scsi_request_fn() and scsi_dispatch_cmd() into scsi_prep_fn(). * CDB_SIZE check in scsi_dispatch_cmd() * SCSI-2 LUN preparation in scsi_dispatch_cmd() * scsi_init_cmd_errh() in scsi_request_fn() No invalid request reaches scsi_request_fn() anymore. This one, I like, there's just one small problem: You can't move scsi_init_cmd_errh() out of the request function path: It's where we set up the sense buffer handling, so it has to be done every time the command is prepared for execution (the prep function is only called once)---think what happens if we turn a command around for retry based on a sense indication. So redo the patch and I'll put it in. Ah.. with later requeue path consolidation patches, all requests get their sense buffer cleared during requeueing, which, IMHO, is more logical. Moving scsi_init_cmd_errh() should come after the patch. Sorry. :-) I'll make another take of this patchset (maybe subset) after issues are resolved. I'll split and reorder relocation of scsi_init_cmd_errh then. Thanks. -- tejun - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/