Re: [PATCH v9 2/3] ahci_xgene: Skip the PHY and clock initialization if already configured by the firmware.
On Thu, Aug 28, 2014 at 02:51:21PM +0530, Suman Tripathi wrote: > This patch implements the feature to skip the PHY and clock > initialization if it is already configured by the firmware. > > Signed-off-by: Loc Ho > Signed-off-by: Suman Tripathi Applied to libata/for-3.17-fixes. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 3/3] ahci_xgene: Fix the link down in first attempt for the APM X-Gene SoC AHCI SATA host controller driver.
On Thu, Aug 28, 2014 at 02:51:22PM +0530, Suman Tripathi wrote: > Due to HW errata the APM X-Gene AHCI SATA host controller reports link > down even if the device presence is detected. This issue is due to speed > negotiation failure. This patch implements the algorithm to retry the > COMRESET if PxSTAT register reports device presence detected but > PHY communication not established. The maximum retry attempts are 3. > > This patch also fixes the code to match the algorithm for the printing > a warning message if the disparity error still exists after link up. > > Signed-off-by: Loc Ho > Signed-off-by: Suman Tripathi Applied to libata/for-3.17-fixes. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 00/20] scsi logging update
One more request: if you already do major surgery to constan.c can we get rid of the CONFIG_SCSI_CONSTANTS all over it? E.g. only keep the code under CONFIG_SCSI_CONSTANTS in, maybe add a no-constants.c for the stubs and move the bit of glue code always compiled to scsi.c? -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 13/20] scsi: consolidate opcode lookup in scsi_opcode_sa_name()
> + *cdb_name = NULL; > + if (cmd >= 0xc0) Can we get a START_VENDOR_SPECIFIC_CMD or similar define for 0xc0, please? Otherwise looks good to me, Reviewed-by: Christoph Hellwig -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] libiscsi: fix potential buffer overrun in __iscsi_conn_send_pdu
On Wed, Sep 03, 2014 at 12:00:39AM -0500, micha...@cs.wisc.edu wrote: > From: Mike Christie > > This patches fixes a potential buffer overrun in __iscsi_conn_send_pdu. > This function is used by iscsi drivers and userspace to send iscsi PDUs/ > commands. For login commands, we have a set buffer size. For all other > commands we do not support data buffers. > > This was reported by Dan Carpenter here: > http://www.spinics.net/lists/linux-scsi/msg66838.html > > Reported-by: Dan Carpenter > Signed-off-by: Mike Christie Looks good, Reviewed-by: Christoph Hellwig -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 21/22] scsi: reduce messages for command failure
On Mon, Sep 01, 2014 at 01:14:23AM +, Elliott, Robert (Server Storage) wrote: > Before this patch set, ratelimited printing was impossible because > there were so many partial-line printk calls. This patch set > may resolve that problem. > > Multiple related lines may still cause problems; if it prints > CDB, sense key, and additional sense code on three lines, you > don't want the CDB and sense key suppressed with the additional > sense code still sneaking out (because command completions > are concurrent). > > If those issues can be overcome, then I'd favor offering ratelimited > calls. Maybe the logging level could be used to select the amount, > like: > 0 = no prints > 1..n = ratelimited versions of the prints > n+1..m = not ratelimited versions of the prints I don't think an unlimited output into the printk buffer is sensible with the number of IOPS we get from modern devices. If you need detailed output you should be using the binary trace buffer. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 20/22] scsi: align logging messages
On Mon, Sep 01, 2014 at 01:00:26AM +, Elliott, Robert (Server Storage) wrote: > > -Original Message- > > From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi- > > ow...@vger.kernel.org] On Behalf Of Christoph Hellwig > > Sent: Sunday, 31 August, 2014 5:26 PM > ... > > On Thu, Aug 28, 2014 at 07:33:34PM +0200, Hannes Reinecke wrote: > > > Always use 'scmd 0x%p' when logging a command pointer. > > > > Is there any good reason to print the address of a command pointer? > > It relates the messages to each other. When you get lots of > interleaved messages like: > > [ 3161.441116] sd 2:0:0:2: [sdt] scmd 88041a874e70 abort scheduled > [ 3161.444258] sd 2:0:0:2: [sdt] scmd 88041a8b3b30 abort scheduled > ... > [ 3162.707427] sd 2:0:0:2: [sdt] aborting command 88041a8b3b30 > [ 3162.710445] sd 2:0:0:2: [sdt] scmd 88041a8b3b30 abort failed, rtn 8195 > [ 3162.713092] sd 2:0:0:2: [sdt] aborting command 88041a8dd530 > [ 3162.715172] sd 2:0:0:2: [sdt] scmd 88041a8dd530 abort failed, rtn 8195 > ... > [ 3171.052277] sd 2:0:0:2: [sdt] scsi_eh_2: flush retry cmd: 88041a8b3b30 > [ 3171.054910] sd 2:0:0:2: [sdt] scsi_eh_2: flush retry cmd: 88041a8dd530 > ... > [ 3203.406218] sd 2:0:0:0: [sdr] scmd 88041a8214b0 not aborting, host in > recovery > [ 3203.406424] sd 2:0:0:0: [sdr] scmd 88041a8474f0 not aborting, host in > recovery > > scmd ties the messages together so you can tell which command > has gotten to which state. grep works. Can we just print the tag instead, that would be a much more human readable number normally. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 20/20] scsi_error: format abort error message
On Wed, Sep 03, 2014 at 12:06:15PM +0200, Hannes Reinecke wrote: > Decode the return value if the command abort failed. I've not seen an answer to my question in reply to the previous version of this. Why would you do pretty printing of a variable that can just return SUCCESS or FAILURE? -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 12/20] scsi: merge print_opcode_name()
On Fri, Sep 05, 2014 at 10:24:40AM +0900, Yoshihiro YUNOMAE wrote: > > #define SA_NAME_LIST_SZ ARRAY_SIZE(sa_names_arr) > > We can delete SA_NAME_LIST_SZ because we define it out of > CONFIG_SCSI_CONSTANTS. I'd generally prefer to just use ARRAY_SIZE() directly instead of adding another layer of indirection. Otherwise looks good, Reviewed-by: Christoph Hellwig -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/20] scsi: Use scsi_print_command() where possible
Looks good, Reviewed-by: Christoph Hellwig -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/20] scsi: use 'bool' as return value for scsi_normalize_sense()
On Wed, Sep 03, 2014 at 12:06:03PM +0200, Hannes Reinecke wrote: > Convert scsi_normalize_sense() and frieds to return 'bool' > instead of an integer. Looks good with the small fix spotted by Yunomae-san. Signed-off-by: Christoph Hellwig -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 09/20] scsi: remove scsi_print_status()
On Wed, Sep 03, 2014 at 12:06:04PM +0200, Hannes Reinecke wrote: > Last caller is gone, so we can remove it. > > Signed-off-by: Hannes Reinecke Looks good, Reviewed-by: Christoph Hellwig -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 07/20] scsi: do not decode sense extras
On Wed, Sep 03, 2014 at 12:06:02PM +0200, Hannes Reinecke wrote: > Currently we're only decoding sense extras for tape devices. > And even there only for fixed format sense formats. > As this is of rather limited use in the general case we should > be stop trying to decode sense extras; the tape driver does > its own decoding anyway. > > Signed-off-by: Hannes Reinecke Looks good, Reviewed-by: Christoph Hellwig -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 06/20] scsi: stop decoding if scsi_normalize_sense() fails
On Wed, Sep 03, 2014 at 12:06:01PM +0200, Hannes Reinecke wrote: > If scsi_normalize_sense() fails we couldn't decode the sense > buffer, and the scsi_sense_hdr fields are invalid. > For those cases we should rather dump the sense buffer > and not try to decode invalid fields. This description still doesn't make sense to me. What you describe is both old and new behavior, you just dump the raw buffer differently. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/20] scsi: introduce sdev_prefix_printk()
On Wed, Sep 03, 2014 at 12:05:59PM +0200, Hannes Reinecke wrote: > Like scmd_printk(), but the device name is passed in as > a string. Can be used by eg ULDs which do not have access > to the scsi_cmnd structure. > > Signed-off-by: Hannes Reinecke Looks good, Reviewed-by: Christoph Hellwig -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/20] aha152x: Debug output update and whitespace cleanup
On Wed, Sep 03, 2014 at 12:05:57PM +0200, Hannes Reinecke wrote: > Remove all uncommented debugging code and move all > printk() statements over to dev_printk(). > And while we're at it we should be doing a whitespace > cleanup, too. Looks good, Reviewed-by: Christoph Hellwig -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] libata-scsi: Update SATL for ZAC drives
On Wed, Jul 30, 2014 at 09:55:10AM +0200, Hannes Reinecke wrote: > ZAC (zoned-access command) drives translate into ZBC (Zoned block > command) device type for SCSI. So implement the correct mappings > into libata-scsi and update the SCSI command set versions. Patch 2-3 look good to me. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] libata: consolidate ata_dev_classify()
Hello, Hannes. Sorry about the delay. On Wed, Jul 30, 2014 at 09:55:08AM +0200, Hannes Reinecke wrote: > ata_dev_classify() just uses the 'lbah' and 'lbam' > fields from the taskfile, so we can as well use those > as arguments and rip out the custom code from sas_ata. I wonder whether it'd easier to just make sas code pass in ata_taskfile instead? The interface which takes three consecutive u8's is kinda error-prone. > --- a/drivers/scsi/aic94xx/aic94xx_task.c > +++ b/drivers/scsi/aic94xx/aic94xx_task.c > @@ -373,10 +373,10 @@ static int asd_build_ata_ascb(struct asd_ascb *ascb, > struct sas_task *task, > > if (unlikely(task->ata_task.device_control_reg_update)) > scb->header.opcode = CONTROL_ATA_DEV; > - else if (dev->sata_dev.command_set == ATA_COMMAND_SET) > - scb->header.opcode = INITIATE_ATA_TASK; > - else > + else if (dev->sata_dev.class == ATA_DEV_ATAPI) > scb->header.opcode = INITIATE_ATAPI_TASK; > + else > + scb->header.opcode = INITIATE_ATA_TASK; Are these changes covered by the patch description? Looks like the patch is mixing two separate logical changes. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
Hey, On Fri, Sep 05, 2014 at 04:22:42PM -0700, Dmitry Torokhov wrote: > > I don't get it. This is a behavior userland already depends on for > > boots. What's there to agree or disagree? This is just a fact that > > we can't do this w/o disturbing some userlands in a major way. > > I am just expressing my disbelief that somebody relies on module loading > being synchronous with probing. Out of curiosity, do you have any > pointers? I've seen initrd scripts which depended on the behavior to wait for storage devices over the years. AFAIK, none of the modern distros does it but this has been such a basic feature all along and it seems highly unlikely to me that there's no userland remaining out there depending on such behavior. We do have a lot of different userlands, many of them quite ad-hoc. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
Hi Tejun, On Sat, Sep 06, 2014 at 07:55:33AM +0900, Tejun Heo wrote: > Hello, Dmitry. > > On Fri, Sep 05, 2014 at 03:49:17PM -0700, Dmitry Torokhov wrote: > > On Sat, Sep 06, 2014 at 07:31:39AM +0900, Tejun Heo wrote: > > > On Sat, Sep 06, 2014 at 07:29:56AM +0900, Tejun Heo wrote: > > > > It is for storage devices which always have guaranteed synchronous > > > > probing on module load and well-defined probing order. > > > > Agree about probing order (IIRC that is why we had to revert the > > wholesale asynchronous probing a few years back) but totally disagree > > about synchronous module loading. > > I don't get it. This is a behavior userland already depends on for > boots. What's there to agree or disagree? This is just a fact that > we can't do this w/o disturbing some userlands in a major way. I am just expressing my disbelief that somebody relies on module loading being synchronous with probing. Out of curiosity, do you have any pointers? > > > Anyway, I just posted a patch that I think preserves module loading > > behavior and solves my issue with built-in modules. It does not help > > Luis' issue though (but then I think the main problem is with systemd > > being stupid there). > > This sure can be worked around from userland side too by not imposing > any timeout on module loading but that said for the same reasons that > you've been arguing until now, I actually do think that it's kinda > silly to make device probing synchronous to module loading at this > time and age. What we disagree on is not that we want to separate > those waits. It is about how to achieve it. Well, there are separate things we want to solve. My main issue is not with modules, but rather compiled-in drivers that stall kernel boot, and these particular drivers are just fine if they are probed out of bound. > > > > To add a bit, if the argument here is that dependency on such behavior > > > shouldn't exist and module loading and device probing should always be > > > asynchronous, the right approach is implementing "synchronous_probing" > > > flag not the other way around. I actually wouldn't hate to see that > > > change happening but whoever submits and routes such a change should > > > be ready for a major shitstorm, I'm afraid. > > > > I think we already had this storm and that is why here we have opt-in > > behavior for the drivers. > > It's a different shitstorm where we actively break bootings on some > userlands. Trust me. That's gonna be a lot worse. That did break bootings and that's why we reverted the wholesale async probing. Thanks. -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
On Fri, Sep 05, 2014 at 04:05:30PM -0700, Arjan van de Ven wrote: > On 9/5/2014 3:52 PM, Dmitry Torokhov wrote: > >On Fri, Sep 05, 2014 at 03:45:08PM -0700, Arjan van de Ven wrote: > >>On 9/5/2014 3:29 PM, Tejun Heo wrote: > >>>Hello, Dmitry. > >>> > >>>On Fri, Sep 05, 2014 at 11:10:03AM -0700, Dmitry Torokhov wrote: > I do not agree that it is actually user-visible change: generally > speaking you > do not really know if device is there or not. They come and go. Like I > said, > consider all permutations, with hot-pluggable buses, deferred probing, > etc, > >>> > >>>It is for storage devices which always have guaranteed synchronous > >>>probing on module load and well-defined probing order. Sure, modern > >>>setups are a lot more dynamic but I'm quite certain that there are > >>>setups in the wild which depend on storage driver loading being > >>>synchronous. We can't simply declare one day that such behavior is > >>>broken and break, most likely, their boots. > >> > >>we even depend on this in the mount-by-label cases > >> > >>many setups assume that the internal storage prevails over the USB stick in > >>the case of conflicts. > >>it's a security issue; you don't want the built in secure bootloader that > >>has a kernel root argument > >>by label/uuid. > >>the security there tends to assume that built-in wins over USB > > > >Ahem... and they sure it works reliably with large storage arrays? With > >SCSI doing probing asynchronously already? > > you tend to trust your large storage array > you tend to not trust the walk up USB stick. If you allow physical access it does not matter really. -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
On 9/5/2014 3:52 PM, Dmitry Torokhov wrote: On Fri, Sep 05, 2014 at 03:45:08PM -0700, Arjan van de Ven wrote: On 9/5/2014 3:29 PM, Tejun Heo wrote: Hello, Dmitry. On Fri, Sep 05, 2014 at 11:10:03AM -0700, Dmitry Torokhov wrote: I do not agree that it is actually user-visible change: generally speaking you do not really know if device is there or not. They come and go. Like I said, consider all permutations, with hot-pluggable buses, deferred probing, etc, It is for storage devices which always have guaranteed synchronous probing on module load and well-defined probing order. Sure, modern setups are a lot more dynamic but I'm quite certain that there are setups in the wild which depend on storage driver loading being synchronous. We can't simply declare one day that such behavior is broken and break, most likely, their boots. we even depend on this in the mount-by-label cases many setups assume that the internal storage prevails over the USB stick in the case of conflicts. it's a security issue; you don't want the built in secure bootloader that has a kernel root argument by label/uuid. the security there tends to assume that built-in wins over USB Ahem... and they sure it works reliably with large storage arrays? With SCSI doing probing asynchronously already? you tend to trust your large storage array you tend to not trust the walk up USB stick. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
Hello, On Fri, Sep 05, 2014 at 03:52:48PM -0700, Dmitry Torokhov wrote: > Ahem... and they sure it works reliably with large storage arrays? With > SCSI doing probing asynchronously already? I believe this has been mentioned before too but, yes, SCSI device probing is asynchronous and parallelized but the registration of the discovered devices are fully serialized according to driver attach order. Storage devices are probed in parallel and attached in a fully deterministic order. That part has never changed. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
Hello, Dmitry. On Fri, Sep 05, 2014 at 03:49:17PM -0700, Dmitry Torokhov wrote: > On Sat, Sep 06, 2014 at 07:31:39AM +0900, Tejun Heo wrote: > > On Sat, Sep 06, 2014 at 07:29:56AM +0900, Tejun Heo wrote: > > > It is for storage devices which always have guaranteed synchronous > > > probing on module load and well-defined probing order. > > Agree about probing order (IIRC that is why we had to revert the > wholesale asynchronous probing a few years back) but totally disagree > about synchronous module loading. I don't get it. This is a behavior userland already depends on for boots. What's there to agree or disagree? This is just a fact that we can't do this w/o disturbing some userlands in a major way. > Anyway, I just posted a patch that I think preserves module loading > behavior and solves my issue with built-in modules. It does not help > Luis' issue though (but then I think the main problem is with systemd > being stupid there). This sure can be worked around from userland side too by not imposing any timeout on module loading but that said for the same reasons that you've been arguing until now, I actually do think that it's kinda silly to make device probing synchronous to module loading at this time and age. What we disagree on is not that we want to separate those waits. It is about how to achieve it. > > To add a bit, if the argument here is that dependency on such behavior > > shouldn't exist and module loading and device probing should always be > > asynchronous, the right approach is implementing "synchronous_probing" > > flag not the other way around. I actually wouldn't hate to see that > > change happening but whoever submits and routes such a change should > > be ready for a major shitstorm, I'm afraid. > > I think we already had this storm and that is why here we have opt-in > behavior for the drivers. It's a different shitstorm where we actively break bootings on some userlands. Trust me. That's gonna be a lot worse. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
On Fri, Sep 05, 2014 at 03:45:08PM -0700, Arjan van de Ven wrote: > On 9/5/2014 3:29 PM, Tejun Heo wrote: > >Hello, Dmitry. > > > >On Fri, Sep 05, 2014 at 11:10:03AM -0700, Dmitry Torokhov wrote: > >>I do not agree that it is actually user-visible change: generally speaking > >>you > >>do not really know if device is there or not. They come and go. Like I said, > >>consider all permutations, with hot-pluggable buses, deferred probing, etc, > > > >It is for storage devices which always have guaranteed synchronous > >probing on module load and well-defined probing order. Sure, modern > >setups are a lot more dynamic but I'm quite certain that there are > >setups in the wild which depend on storage driver loading being > >synchronous. We can't simply declare one day that such behavior is > >broken and break, most likely, their boots. > > we even depend on this in the mount-by-label cases > > many setups assume that the internal storage prevails over the USB stick in > the case of conflicts. > it's a security issue; you don't want the built in secure bootloader that has > a kernel root argument > by label/uuid. > the security there tends to assume that built-in wins over USB Ahem... and they sure it works reliably with large storage arrays? With SCSI doing probing asynchronously already? Thanks. -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
On 9/5/2014 3:29 PM, Tejun Heo wrote: Hello, Dmitry. On Fri, Sep 05, 2014 at 11:10:03AM -0700, Dmitry Torokhov wrote: I do not agree that it is actually user-visible change: generally speaking you do not really know if device is there or not. They come and go. Like I said, consider all permutations, with hot-pluggable buses, deferred probing, etc, It is for storage devices which always have guaranteed synchronous probing on module load and well-defined probing order. Sure, modern setups are a lot more dynamic but I'm quite certain that there are setups in the wild which depend on storage driver loading being synchronous. We can't simply declare one day that such behavior is broken and break, most likely, their boots. we even depend on this in the mount-by-label cases many setups assume that the internal storage prevails over the USB stick in the case of conflicts. it's a security issue; you don't want the built in secure bootloader that has a kernel root argument by label/uuid. the security there tends to assume that built-in wins over USB -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
Hello, Luis. On Fri, Sep 05, 2014 at 11:12:17AM -0700, Luis R. Rodriguez wrote: > Meanwhile we are allowing a major design consideration such as a 30 > second timeout for both init + probe all of a sudden become a hard > requirement for device drivers. I see your point but can't also be > introducing major design changes willy nilly either. We *need* a > solution for the affected drivers. Yes, make the behavior specifically specified from userland. When did I ever say that there should be no solution for the problem? I've been saying that the behavior should be selected from userland from the get-go, haven't I? I have no idea how the seleciton should be. It could be per-insmod or maybe just a system-wide flag with explicit exceptions marked on drivers is good enough. I don't know. > Also what stops drivers from going ahead and just implementing their > own async probe? Would that now be frowned upon as it strives away The drivers can't. How many times should I explain the same thing over and over again. libata can't simply make probing asynchronous w.r.t. module loading no matter how it does it. Yeah, sure, there can be other drivers which can do that without most people noticing it but a storage driver isn't one of them and the storage drivers are the problematic ones already, right? > from the original design? The bool would let those drivers do this > easily, and we would still need to identify these drivers, although > this particular change can be NAK'd Oleg's suggestion on > WARN_ON(fatal_signal_pending() at the end of load_module() seems to me > at least needed. And if its not async probe... what do those with > failed drivers do? I'm getting tired of explaining the same thing over and over again. The said change was nacked because the whole approach of "let's see which drivers get reported on the issue which exists basically for all drivers and just change the behavior of them" is braindead. It makes no sense whatsoever. It doesn't address the root cause of the problem while making the same class of drivers behave significantly differently for no good reason. Please stop chasing your own tail and try to understand the larger picture. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
On Sat, Sep 06, 2014 at 07:29:56AM +0900, Tejun Heo wrote: > It is for storage devices which always have guaranteed synchronous > probing on module load and well-defined probing order. Sure, modern > setups are a lot more dynamic but I'm quite certain that there are > setups in the wild which depend on storage driver loading being > synchronous. We can't simply declare one day that such behavior is > broken and break, most likely, their boots. To add a bit, if the argument here is that dependency on such behavior shouldn't exist and module loading and device probing should always be asynchronous, the right approach is implementing "synchronous_probing" flag not the other way around. I actually wouldn't hate to see that change happening but whoever submits and routes such a change should be ready for a major shitstorm, I'm afraid. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
Hello, Dmitry. On Fri, Sep 05, 2014 at 11:10:03AM -0700, Dmitry Torokhov wrote: > I do not agree that it is actually user-visible change: generally speaking you > do not really know if device is there or not. They come and go. Like I said, > consider all permutations, with hot-pluggable buses, deferred probing, etc, It is for storage devices which always have guaranteed synchronous probing on module load and well-defined probing order. Sure, modern setups are a lot more dynamic but I'm quite certain that there are setups in the wild which depend on storage driver loading being synchronous. We can't simply declare one day that such behavior is broken and break, most likely, their boots. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 2/6] driver-core: add driver async_probe support
Hi Luis,
On Thu, Sep 04, 2014 at 11:37:23PM -0700, Luis R. Rodriguez wrote:
> 1) when a built-in driver takes a few seconds to initialize its
>delays can stall the overall boot process
This patch does not solve the 2nd issue fully as it only calls probe
asynchronously during driver registration (and also only for modules???
- it checks drv->owner in a few places). The device may get created
after driver is initialized, in this case we still want probe to be
called asynchronously.
I think something like the patch below should work. Note that it uses
async_checdule(), so that will satisy for the moment Tejun's objections
to the behavior with regard to module loading and initialization, but it
does not solve your issue with modules being killed after 30 seconds.
To tell the truth I think systemd should not be doing it; it is not its
place to dictate how long module should take to load. It may print
warnings and we'll work on fixing the drivers, but aborting boot just
because they feel like it took too long is not a good idea.
Thanks.
--
Dmitry
driver-core: add driver async_probe support
From: Dmitry Torokhov
Some devices take a long time when initializing, and not all drivers are
suited to initialize their devices when they are open. For example, input
drivers need to interrogate device in order to publish its capabilities
before userspace will open them. When such drivers are compiled into kernel
they may stall entire kernel initialization.
This change allows drivers request for their probe functions to be called
asynchronously during driver and device registration (manual binding is
still synchronous). Because async_schedule is used to perform asynchronous
calls module loading will still wait for the probing to complete.
This is based on earlier patch by "Luis R. Rodriguez"
Signed-off-by: Dmitry Torokhov
---
drivers/base/bus.c | 31 ++
drivers/base/dd.c | 106 +++-
include/linux/device.h |2 +
3 files changed, 112 insertions(+), 27 deletions(-)
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 83e910a..49fe573 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -10,6 +10,7 @@
*
*/
+#include
#include
#include
#include
@@ -547,15 +548,12 @@ void bus_probe_device(struct device *dev)
{
struct bus_type *bus = dev->bus;
struct subsys_interface *sif;
- int ret;
if (!bus)
return;
- if (bus->p->drivers_autoprobe) {
- ret = device_attach(dev);
- WARN_ON(ret < 0);
- }
+ if (bus->p->drivers_autoprobe)
+ device_initial_probe(dev);
mutex_lock(&bus->p->mutex);
list_for_each_entry(sif, &bus->p->interfaces, node)
@@ -657,6 +655,17 @@ static ssize_t uevent_store(struct device_driver *drv,
const char *buf,
}
static DRIVER_ATTR_WO(uevent);
+static void driver_attach_async(void *_drv, async_cookie_t cookie)
+{
+ struct device_driver *drv = _drv;
+ int ret;
+
+ ret = driver_attach(drv);
+
+ pr_debug("bus: '%s': driver %s async attach completed: %d\n",
+drv->bus->name, drv->name, ret);
+}
+
/**
* bus_add_driver - Add a driver to the bus.
* @drv: driver.
@@ -689,9 +698,15 @@ int bus_add_driver(struct device_driver *drv)
klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers);
if (drv->bus->p->drivers_autoprobe) {
- error = driver_attach(drv);
- if (error)
- goto out_unregister;
+ if (drv->async_probe) {
+ pr_debug("bus: '%s': probing driver %s
asynchronously\n",
+ drv->bus->name, drv->name);
+ async_schedule(driver_attach_async, drv);
+ } else {
+ error = driver_attach(drv);
+ if (error)
+ goto out_unregister;
+ }
}
module_add_driver(drv->owner, drv);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index e4ffbcf..67a2f85 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -402,31 +402,52 @@ int driver_probe_device(struct device_driver *drv, struct
device *dev)
return ret;
}
-static int __device_attach(struct device_driver *drv, void *data)
+struct device_attach_data {
+ struct device *dev;
+ bool check_async;
+ bool want_async;
+ bool have_async;
+};
+
+static int __device_attach_driver(struct device_driver *drv, void *_data)
{
- struct device *dev = data;
+ struct device_attach_data *data = _data;
+ struct device *dev = data->dev;
if (!driver_match_device(drv, dev))
return 0;
+ if (drv->async_probe)
+ data->have_async = true;
+
+ if (data->check_async && drv->async_probe != data->want_async)
+ return 0;
+
return driver_
Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
On Fri, Sep 05, 2014 at 11:12:17AM -0700, Luis R. Rodriguez wrote: > On Fri, Sep 5, 2014 at 10:49 AM, Tejun Heo wrote: > > Hello, > > > > On Fri, Sep 05, 2014 at 09:44:05AM -0700, Dmitry Torokhov wrote: > >> Which problem are we talking about here though? It does solve the slow > >> device > >> stalling the rest if the kernel booting (non-module case) for me. > > > > The other one. The one with timeout. Neither cxgb4 or pata_marvell > > has slow probing stalling boot problem. > > > >> I also reject the notion that anyone should be relying on drivers to be > >> fully > >> bound on module loading. It is not nineties anymore. We have hot pluggable > >> buses, deferred probing, and even for not hot-pluggable ones the module > >> providing the device itself might not be yet loaded. Any scripts that > >> expect to > >> find device 100% ready after module loading are simply broken. > > > > We've been treating loading + probing as a single operation when > > loading drivers and the assumption has always been that the existing > > devices at the time of loading finished probing by the time insmod > > finishes. We now need to split loading and probing and wait for each > > of them differently. The *only* thing we can do is somehow making the > > issuer specify that it's gonna wait for probing separately. I'm not > > sure this can even be up for discussion. We're talking about a major > > userland visible behavior change. We simply can't change it > > underneath the existing users. > > Meanwhile we are allowing a major design consideration such as a 30 > second timeout for both init + probe all of a sudden become a hard > requirement for device drivers. I see your point but can't also be > introducing major design changes willy nilly either. We *need* a > solution for the affected drivers. > > Also what stops drivers from going ahead and just implementing their > own async probe? They already do and the problem is that they do that poorly. One of the issues is that the device is considered bound and so may attempt to suspend/resume them, or unbind them, and the driver is not ready for such operations to take place. And even though driver is bound "synchronously" it does not help the user in the slightest and the object that is the result of driver initialization is still created asynchronously and is not ready (well, it might if drivers use async_schedule as we are doing asych_sycnhronize_full() in module load.unload). Thanks. -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
On Fri, Sep 5, 2014 at 10:49 AM, Tejun Heo wrote: > Hello, > > On Fri, Sep 05, 2014 at 09:44:05AM -0700, Dmitry Torokhov wrote: >> Which problem are we talking about here though? It does solve the slow device >> stalling the rest if the kernel booting (non-module case) for me. > > The other one. The one with timeout. Neither cxgb4 or pata_marvell > has slow probing stalling boot problem. > >> I also reject the notion that anyone should be relying on drivers to be fully >> bound on module loading. It is not nineties anymore. We have hot pluggable >> buses, deferred probing, and even for not hot-pluggable ones the module >> providing the device itself might not be yet loaded. Any scripts that expect >> to >> find device 100% ready after module loading are simply broken. > > We've been treating loading + probing as a single operation when > loading drivers and the assumption has always been that the existing > devices at the time of loading finished probing by the time insmod > finishes. We now need to split loading and probing and wait for each > of them differently. The *only* thing we can do is somehow making the > issuer specify that it's gonna wait for probing separately. I'm not > sure this can even be up for discussion. We're talking about a major > userland visible behavior change. We simply can't change it > underneath the existing users. Meanwhile we are allowing a major design consideration such as a 30 second timeout for both init + probe all of a sudden become a hard requirement for device drivers. I see your point but can't also be introducing major design changes willy nilly either. We *need* a solution for the affected drivers. Also what stops drivers from going ahead and just implementing their own async probe? Would that now be frowned upon as it strives away from the original design? The bool would let those drivers do this easily, and we would still need to identify these drivers, although this particular change can be NAK'd Oleg's suggestion on WARN_ON(fatal_signal_pending() at the end of load_module() seems to me at least needed. And if its not async probe... what do those with failed drivers do? Luis -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
On Sat, Sep 06, 2014 at 02:49:25AM +0900, Tejun Heo wrote: > Hello, > > On Fri, Sep 05, 2014 at 09:44:05AM -0700, Dmitry Torokhov wrote: > > Which problem are we talking about here though? It does solve the slow > > device > > stalling the rest if the kernel booting (non-module case) for me. > > The other one. The one with timeout. Neither cxgb4 or pata_marvell > has slow probing stalling boot problem. > > > I also reject the notion that anyone should be relying on drivers to be > > fully > > bound on module loading. It is not nineties anymore. We have hot pluggable > > buses, deferred probing, and even for not hot-pluggable ones the module > > providing the device itself might not be yet loaded. Any scripts that > > expect to > > find device 100% ready after module loading are simply broken. > > We've been treating loading + probing as a single operation when > loading drivers and the assumption has always been that the existing > devices at the time of loading finished probing by the time insmod > finishes. We now need to split loading and probing and wait for each > of them differently. The *only* thing we can do is somehow making the > issuer specify that it's gonna wait for probing separately. I'm not > sure this can even be up for discussion. We're talking about a major > userland visible behavior change. I do not agree that it is actually user-visible change: generally speaking you do not really know if device is there or not. They come and go. Like I said, consider all permutations, with hot-pluggable buses, deferred probing, etc, etc. Thanks. -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
Hello, On Fri, Sep 05, 2014 at 09:44:05AM -0700, Dmitry Torokhov wrote: > Which problem are we talking about here though? It does solve the slow device > stalling the rest if the kernel booting (non-module case) for me. The other one. The one with timeout. Neither cxgb4 or pata_marvell has slow probing stalling boot problem. > I also reject the notion that anyone should be relying on drivers to be fully > bound on module loading. It is not nineties anymore. We have hot pluggable > buses, deferred probing, and even for not hot-pluggable ones the module > providing the device itself might not be yet loaded. Any scripts that expect > to > find device 100% ready after module loading are simply broken. We've been treating loading + probing as a single operation when loading drivers and the assumption has always been that the existing devices at the time of loading finished probing by the time insmod finishes. We now need to split loading and probing and wait for each of them differently. The *only* thing we can do is somehow making the issuer specify that it's gonna wait for probing separately. I'm not sure this can even be up for discussion. We're talking about a major userland visible behavior change. We simply can't change it underneath the existing users. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
WARNING in block layer triggered in 3.17-rc3
James and Jens: I got a WARNING when unbinding the sd driver from a USB flash drive and then binding it back again. Here's where the flash drive gets probed initially: [ 143.300886] usb-storage 4-8:1.0: usb_probe_interface [ 143.300911] usb-storage 4-8:1.0: usb_probe_interface - got id [ 143.300930] usb-storage 4-8:1.0: USB Mass Storage device detected [ 143.318239] scsi host0: usb-storage 4-8:1.0 [ 143.359979] scsi 0:0:0:0: Direct-Access Ut165USB2FlashStorage 0.00 PQ: 0 ANSI: 2 [ 143.376366] usbcore: registered new interface driver usb-storage [ 143.468464] sd 0:0:0:0: [sda] 7892040 512-byte logical blocks: (4.04 GB/3.76 GiB) [ 143.481725] sd 0:0:0:0: [sda] Write Protect is off [ 143.485712] sd 0:0:0:0: [sda] Mode Sense: 00 00 00 00 [ 143.487064] sd 0:0:0:0: [sda] Asking for cache data failed [ 143.498428] sd 0:0:0:0: [sda] Assuming drive cache: write through [ 143.656797] sda: sda1 [ 143.676922] sd 0:0:0:0: [sda] Attached SCSI removable disk Then I did echo 0:0:0:0 >/sys/bus/scsi/drivers/sd_mod/unbind followed by echo 0:0:0:0 >/sys/bus/scsi/drivers/sd_mod/bind which resulted in: [ 165.079557] sd 0:0:0:0: [sda] 7892040 512-byte logical blocks: (4.04 GB/3.76 GiB) [ 165.093510] sd 0:0:0:0: [sda] Write Protect is off [ 165.104388] sd 0:0:0:0: [sda] Mode Sense: 00 00 00 00 [ 165.105632] sd 0:0:0:0: [sda] Asking for cache data failed [ 165.115136] sd 0:0:0:0: [sda] Assuming drive cache: write through [ 165.142950] sda: sda1 [ 165.156480] [ cut here ] [ 165.159912] WARNING: CPU: 0 PID: 29 at block/blk-core.c:473 blk_queue_bypass_end+0x4d/0x62() [ 165.160030] Modules linked in: sd_mod usb_storage scsi_mod hid_generic usbhid hid pcspkr evdev i915 cfbfillrect cfbimgblt i2c_algo_bit cfbcopyarea video fbcon backlight bitblit softcursor font ehci_pci drm_kms_helper uhci_hcd ehci_hcd ohci_pci ohci_hcd drm i2ccore usbcore e100 mii usb_common fb fbdev fan processor button thermal_sys [ 165.160030] CPU: 0 PID: 29 Comm: kworker/u4:1 Not tainted 3.17.0-rc3AS-dirty #12 [ 165.160030] Hardware name: Hewlett-Packard HP dx2000 MT (EE004AA)/08FCh, BIOS 1.1711/24/2005 [ 165.160030] Workqueue: events_unbound async_run_entry_fn [ 165.160030] c105d2ef ea569e10 c12771c0 ea569e28 c102c5fb c114907d [ 165.160030] ecc18000 ea619c20 ecc18000 ea569e38 c102c676 0009 ea569e44 [ 165.160030] c114907d ea619c20 ea569e5c c114b97d ea569e5c ea619c20 ed74e400 ea619c2c [ 165.160030] Call Trace: [ 165.160030] [] ? console_unlock+0x37e/0x3ab [ 165.160030] [] dump_stack+0x49/0x73 [ 165.160030] [] warn_slowpath_common+0x5c/0x73 [ 165.160030] [] ? blk_queue_bypass_end+0x4d/0x62 [ 165.160030] [] warn_slowpath_null+0xf/0x13 [ 165.160030] [] blk_queue_bypass_end+0x4d/0x62 [ 165.160030] [] blk_register_queue+0x8f/0xc4 [ 165.160030] [] add_disk+0x2bc/0x3a8 [ 165.160030] [] sd_probe_async+0xf5/0x17b [sd_mod] [ 165.160030] [] async_run_entry_fn+0x59/0xf9 [ 165.160030] [] process_one_work+0x187/0x2ac [ 165.160030] [] ? process_one_work+0x129/0x2ac [ 165.160030] [] worker_thread+0x1b1/0x26b [ 165.160030] [] ? process_scheduled_works+0x21/0x21 [ 165.160030] [] kthread+0x82/0x87 [ 165.160030] [] ? _raw_spin_unlock_irq+0x22/0x3f [ 165.160030] [] ? radix_tree_tag_set+0x3f/0xa5 [ 165.160030] [] ret_from_kernel_thread+0x21/0x30 [ 165.160030] [] ? __kthread_parkme+0x50/0x50 [ 165.160030] ---[ end trace 31df765b6ea80892 ]--- [ 165.308986] sd 0:0:0:0: [sda] Attached SCSI removable disk I don't know what's going on here, but it looks like something doesn't get cleaned up properly during the unbind operation. This was in vanilla 3.17-rc3. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
On Fri, Sep 05, 2014 at 12:59:49PM +0200, Oleg Nesterov wrote: > On 09/04, Luis R. Rodriguez wrote: > > > > From: "Luis R. Rodriguez" > > > > The new umh kill option has allowed kthreads to receive > > kill signals but they are generally accepting all sources > > of kill signals > > And I think this is right, > > > while the original motivation was to enable > > through the OOM from sending the kill. > > even if the main concern was OOM. > > > Users can provide a log output and it should be clear on > > the trace what probe / driver got the kill signal. > > Well, if you need a WARN output, perhaps you could just add > WARN_ON(fatal_signal_pending()) at the end of load_module() ? We could and that's a good idea, thanks! This however would at least allow the device to be functional in the case the kill was received during kthread usage, but it would certainly also set precedents for doing similar things in the kernel which I do agree with is hacky. If we had upstream at least WARN_ON(fatal_signal_pending()) as you note then I think it would at least be a reasonable compromise. > Not only kthread_create() can fail if systemd sends SIGKILL. Sure, although its currently the only source found and debugged. > > Although Oleg had rejected a > > similar change a while ago > > And honestly, I still dislike this change. Don't blame you. The code is sensitive and hacky. Luis -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Problem with USB-to-SATA adapters (was: AS2105-based enclosure size issues with >2TB HDDs)
On Wed, 3 Sep 2014, James Bottomley wrote:
> On Wed, 2014-09-03 at 16:30 -0400, Alan Stern wrote:
> > On Wed, 3 Sep 2014, James Bottomley wrote:
> >
> > > Before we embark on elaborate hacks, why don't we just make the capacity
> > > writeable (by root) in sysfs from userspace (will require block change)?
> > > We can then encode all the nasty heuristics (including gpt reading) in
> > > userspace as a udev rule.
> >
> > That's what I'm working on. Except that I don't know where to do it in
> > the block layer, so for now I'm adding the attribute to sd.c.
> >
> > Where in the block layer would the right place be? We want this to
> > apply only to entire devices, not to individual partitions.
>
> The bottom layer for this is part0.nr_sects which is the size attribute
> you see in the block sysfs. However, it looks like we keep a separate
> value in sdkp, but we don't ever seem to use it (except to see if the
> capacity has changed).
>
> So this could be done in two ways: add a writeable capacity attribute in
> sd.c, as you were originally thinking (it would call set_capacity() on
> write and that would update the block layer) or make the size parameter
> writeable.
Here's my patch to the sd driver. It seems to work -- writing to the
capacity_override attribute does change the device size. But then you
have to force the kernel to re-read the partition table manually, for
example by running
blockdev --rereadpt /dev/sdX
because I couldn't see any reasonable way to make this happen
automatically.
Alan Stern
Index: usb-3.17/drivers/scsi/sd.h
===
--- usb-3.17.orig/drivers/scsi/sd.h
+++ usb-3.17/drivers/scsi/sd.h
@@ -66,6 +66,7 @@ struct scsi_disk {
struct gendisk *disk;
atomic_topeners;
sector_tcapacity; /* size in 512-byte sectors */
+ sector_tcapacity_override; /* in native-size blocks */
u32 max_xfer_blocks;
u32 max_ws_blocks;
u32 max_unmap_blocks;
Index: usb-3.17/drivers/scsi/sd.c
===
--- usb-3.17.orig/drivers/scsi/sd.c
+++ usb-3.17/drivers/scsi/sd.c
@@ -477,6 +477,38 @@ max_write_same_blocks_store(struct devic
}
static DEVICE_ATTR_RW(max_write_same_blocks);
+static ssize_t
+capacity_override_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct scsi_disk *sdkp = to_scsi_disk(dev);
+
+ return sprintf(buf, "%llu\n",
+ (unsigned long long) sdkp->capacity_override);
+}
+
+static ssize_t
+capacity_override_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct scsi_disk *sdkp = to_scsi_disk(dev);
+ unsigned long long cap;
+ int err;
+
+ if (!capable(CAP_SYS_ADMIN))
+ return -EACCES;
+
+ err = kstrtoull(buf, 10, &cap);
+ if (err)
+ return err;
+
+ sdkp->capacity_override = cap;
+ revalidate_disk(sdkp->disk);
+
+ return count;
+}
+static DEVICE_ATTR_RW(capacity_override);
+
static struct attribute *sd_disk_attrs[] = {
&dev_attr_cache_type.attr,
&dev_attr_FUA.attr,
@@ -489,6 +521,7 @@ static struct attribute *sd_disk_attrs[]
&dev_attr_provisioning_mode.attr,
&dev_attr_max_write_same_blocks.attr,
&dev_attr_max_medium_access_timeouts.attr,
+ &dev_attr_capacity_override.attr,
NULL,
};
ATTRIBUTE_GROUPS(sd_disk);
@@ -2158,6 +2191,13 @@ sd_read_capacity(struct scsi_disk *sdkp,
struct scsi_device *sdp = sdkp->device;
sector_t old_capacity = sdkp->capacity;
+ /* Did the user override the reported capacity? */
+ if (!sdkp->first_scan && sdkp->capacity_override) {
+ sector_size = sdkp->device->sector_size;
+ sdkp->capacity = sdkp->capacity_override;
+ goto got_data;
+ }
+
if (sd_try_rc16_first(sdp)) {
sector_size = read_capacity_16(sdkp, sdp, buffer);
if (sector_size == -EOVERFLOW)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 2/6] driver-core: add driver async_probe support
On Fri, Sep 05, 2014 at 01:24:17PM +0200, Oleg Nesterov wrote:
> On 09/04, Luis R. Rodriguez wrote:
> >
> > struct driver_private {
> > struct kobject kobj;
> > struct klist klist_devices;
> > struct klist_node knode_bus;
> > struct module_kobject *mkobj;
> > + struct driver_attach_work *attach_work;
> > struct device_driver *driver;
>
> I am not arguing, just curious...
>
> Are you trying to shrink sizeof(driver_private) ?
Yeap.
> The code can be simpler
> if you just embedd "struct work_struct attach_work" into driver_private,
> and you do not need "struct driver_attach_work" or another ->driver pointer
> this way.
Agreed, I considered it and figured it wouldn't make much sense
to push onto folks more bytes if this feature was optional and
likely only used by a few drivers, so a pointer / kzalloc seemed
better to deal with. This saves us 24 bytes. I even tried to
implement a container_of_p() for pointers but that obviosly
didn't work well fast as a pointer can have any address and is
not relative to the parent, and if its on stack the address
can vary depending on implementation. For example the first member
should always have the same address as the struct but if the
first member is a pointer it would be off for me by 12 bytes.
I am not sure if this is standarized or not.
Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
On Friday, September 05, 2014 11:12:41 PM Tejun Heo wrote: > On Fri, Sep 05, 2014 at 12:47:16AM -0700, Luis R. Rodriguez wrote: > > Ah -- well without it the way we "find" drivers that need this new > > "async feature" is by a bug report and folks saying their system can't > > boot, or they say their device doesn't come up. That's all. Tracing > > this to systemd and a timeout was one of the most ugliest things ever. > > There two insane bug reports you can go check: > > > > mptsas was the first: > > > > http://article.gmane.org/gmane.linux.kernel/1669550 > > https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1297248 > > > > Then cxgb4: > > > > https://bugzilla.novell.com/show_bug.cgi?id=877622 > > > > I only had Cc'd you on the newest gem pata_marvell : > > > > https://bugzilla.kernel.org/show_bug.cgi?id=59581 > > > > We can't seriously expect to be doing all this work for every driver. > > a WARN_ONCE() would enable us to find the drivers that need this new > > async probe "feature". > > This whole approach of trying to mark specific drivers as needing > "async probing" is completely broken for the problem at hand. It > can't address the problem adequately while breaking backward > compatibility. I don't think this makes much sense. > Which problem are we talking about here though? It does solve the slow device stalling the rest if the kernel booting (non-module case) for me. I also reject the notion that anyone should be relying on drivers to be fully bound on module loading. It is not nineties anymore. We have hot pluggable buses, deferred probing, and even for not hot-pluggable ones the module providing the device itself might not be yet loaded. Any scripts that expect to find device 100% ready after module loading are simply broken. Thanks. -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] mpt2sas: remove duplicate disable_discovery MODULE_PARAM
The disable_discovery module parameter is declared and only used by mpt2sas_scsih.c. Remove the extra copy in mpt2sas_base.c. Signed-off-by: Joe Lawrence --- drivers/scsi/mpt2sas/mpt2sas_base.c |4 1 file changed, 4 deletions(-) diff --git a/drivers/scsi/mpt2sas/mpt2sas_base.c b/drivers/scsi/mpt2sas/mpt2sas_base.c index cf51b48..bb8dcb0 100644 --- a/drivers/scsi/mpt2sas/mpt2sas_base.c +++ b/drivers/scsi/mpt2sas/mpt2sas_base.c @@ -84,10 +84,6 @@ static int mpt2sas_fwfault_debug; MODULE_PARM_DESC(mpt2sas_fwfault_debug, " enable detection of firmware fault " "and halt firmware - (default=0)"); -static int disable_discovery = -1; -module_param(disable_discovery, int, 0); -MODULE_PARM_DESC(disable_discovery, " disable discovery "); - /** * _scsih_set_fwfault_debug - global setting of ioc->fwfault_debug. * -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi_debug: deadlock between completions and surprise module removal
On 09/05/14 15:56, Douglas Gilbert wrote: With scsi-mq I think many LLDs probably have a new race possibility between a surprise rmmod of the LLD and another thread presenting a new command at about the same time (or another thread's command completing around that time). Does anything above the LLD stop this happening? Looking at mpt3sas and hpsa module exit calls, they don't seem to guard against this possibility. The test is pretty easy: build the LLD as a module, load it and fire up a multi-thread, libaio fio test on one or more devices (SSDs would probably be good) on that LLD. While the test is running, do 'rmmod LLD'. An LLD must call scsi_remove_host() directly or indirectly from the module cleanup path. scsi_remove_host() triggers a call to blk_cleanup_queue(). That last function sets the flag QUEUE_FLAG_DYING which prevents that new I/O is queued and waits until previously queued requests have finished before returning. Bart. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
On Fri, Sep 05, 2014 at 12:47:16AM -0700, Luis R. Rodriguez wrote: > Ah -- well without it the way we "find" drivers that need this new > "async feature" is by a bug report and folks saying their system can't > boot, or they say their device doesn't come up. That's all. Tracing > this to systemd and a timeout was one of the most ugliest things ever. > There two insane bug reports you can go check: > > mptsas was the first: > > http://article.gmane.org/gmane.linux.kernel/1669550 > https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1297248 > > Then cxgb4: > > https://bugzilla.novell.com/show_bug.cgi?id=877622 > > I only had Cc'd you on the newest gem pata_marvell : > > https://bugzilla.kernel.org/show_bug.cgi?id=59581 > > We can't seriously expect to be doing all this work for every driver. > a WARN_ONCE() would enable us to find the drivers that need this new > async probe "feature". This whole approach of trying to mark specific drivers as needing "async probing" is completely broken for the problem at hand. It can't address the problem adequately while breaking backward compatibility. I don't think this makes much sense. Nacked-by: Tejun Heo Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/19] lpfc 10.4.8000.0: Update lpfc version to driver version 10.4.8000.0
On Fri, 2014-09-05 at 09:31 -0400, James Smart wrote: > On 9/5/2014 1:35 AM, Christoph Hellwig wrote: > > - patches that you send on with your maintainer hat on should be > > signed off by you, not just reviewed. > > ok - but I guess I had a different interpretation of the meaning for > signed-by. I thought it conveyed an ownership and originality of > authorship of the content posted. As such, if I didn't contribute > anything in the patch, I shouldn't give anything other than a > reviewed-by indicating approval. No, this is a DCO thing;the Signed-off-by: tag is the certification under the DCO: http://developercertificate.org/ It follows the chain of transmission, whether you alter the patch or not, so it needs your Signed-off-by for any patch you send on behalf of another (whether or not you alter it). By convention, you also append your alterations to the change log in square brackets, but you don't have to bother if it's trivial. James -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] scsi_debug: deadlock between completions and surprise module removal
With scsi-mq I think many LLDs probably have a new
race possibility between a surprise rmmod of the LLD
and another thread presenting a new command at about
the same time (or another thread's command completing
around that time). Does anything above the LLD stop
this happening?
Looking at mpt3sas and hpsa module exit calls, they don't
seem to guard against this possibility.
The test is pretty easy: build the LLD as a module, load
it and fire up a multi-thread, libaio fio test on one or
more devices (SSDs would probably be good) on that LLD.
While the test is running, do 'rmmod LLD'.
Doug Gilbert
On 14-09-05 01:24 AM, Christoph Hellwig wrote:
Can I get another review for this one?
On Sun, Aug 31, 2014 at 07:09:59PM -0400, Douglas Gilbert wrote:
A deadlock has been reported when the completion
of SCSI commands (simulated by a timer) was surprised
by a module removal. This patch removes one half of
the offending locks around timer deletions. This fix
is applied both to stop_all_queued() which is were
the deadlock was discovered and stop_queued_cmnd()
which has very similar logic.
This patch should be applied both to the lk 3.17 tree
and Christoph's drivers-for-3.18 tree.
Tested-and-reported-by: Milan Broz
Signed-off-by: Douglas Gilbert
--- a/drivers/scsi/scsi_debug.c 2014-08-26 13:24:51.646948507 -0400
+++ b/drivers/scsi/scsi_debug.c 2014-08-30 18:04:54.589226679 -0400
@@ -2743,6 +2743,13 @@ static int stop_queued_cmnd(struct scsi_
if (test_bit(k, queued_in_use_bm)) {
sqcp = &queued_arr[k];
if (cmnd == sqcp->a_cmnd) {
+ devip = (struct sdebug_dev_info *)
+ cmnd->device->hostdata;
+ if (devip)
+ atomic_dec(&devip->num_in_q);
+ sqcp->a_cmnd = NULL;
+ spin_unlock_irqrestore(&queued_arr_lock,
+ iflags);
if (scsi_debug_ndelay > 0) {
if (sqcp->sd_hrtp)
hrtimer_cancel(
@@ -2755,18 +2762,13 @@ static int stop_queued_cmnd(struct scsi_
if (sqcp->tletp)
tasklet_kill(sqcp->tletp);
}
- __clear_bit(k, queued_in_use_bm);
- devip = (struct sdebug_dev_info *)
- cmnd->device->hostdata;
- if (devip)
- atomic_dec(&devip->num_in_q);
- sqcp->a_cmnd = NULL;
- break;
+ clear_bit(k, queued_in_use_bm);
+ return 1;
}
}
}
spin_unlock_irqrestore(&queued_arr_lock, iflags);
- return (k < qmax) ? 1 : 0;
+ return 0;
}
/* Deletes (stops) timers or tasklets of all queued commands */
@@ -2782,6 +2784,13 @@ static void stop_all_queued(void)
if (test_bit(k, queued_in_use_bm)) {
sqcp = &queued_arr[k];
if (sqcp->a_cmnd) {
+ devip = (struct sdebug_dev_info *)
+ sqcp->a_cmnd->device->hostdata;
+ if (devip)
+ atomic_dec(&devip->num_in_q);
+ sqcp->a_cmnd = NULL;
+ spin_unlock_irqrestore(&queued_arr_lock,
+ iflags);
if (scsi_debug_ndelay > 0) {
if (sqcp->sd_hrtp)
hrtimer_cancel(
@@ -2794,12 +2803,8 @@ static void stop_all_queued(void)
if (sqcp->tletp)
tasklet_kill(sqcp->tletp);
}
- __clear_bit(k, queued_in_use_bm);
- devip = (struct sdebug_dev_info *)
- sqcp->a_cmnd->device->hostdata;
- if (devip)
- atomic_dec(&devip->num_in_q);
- sqcp->a_cmnd = NULL;
+ clear_bit(k, queued_in_use_bm);
+ spin_lock_irqsave(&queued_arr_lock, iflags);
}
}
}
---end quoted text---
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo
Re: [PATCH 0/19] lpfc 10.4.8000.0: Update lpfc version to driver version 10.4.8000.0
no problem, although if these rules were published, I would have tried to do so earlier. although - I do have a couple of questions. On 9/5/2014 1:35 AM, Christoph Hellwig wrote: I've applied the series, but for next time can you make sure to follow the proper format: - remove the version number in every subject line yeah - it's long - patches you resend from an original author should be unchanged, except that the From: lines moves into the mail body So.. you do not want me to resolve merge conflicts or fuzz before posting ? note: content did not change - only merged into my git tree and re-cut them. - patches that you send on with your maintainer hat on should be signed off by you, not just reviewed. ok - but I guess I had a different interpretation of the meaning for signed-by. I thought it conveyed an ownership and originality of authorship of the content posted. As such, if I didn't contribute anything in the patch, I shouldn't give anything other than a reviewed-by indicating approval. not strictly required, but making my life a lot easier would be if all patches are sent by reply to the original mail. git-send-email does this, and it seems like the Emulex division supporting be2scsi has found a way to use it with the corporate email servers. yep - although different countries have different logistics. But, I should be able to get around this. -- james -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 2/6] driver-core: add driver async_probe support
On 09/04, Luis R. Rodriguez wrote:
>
> struct driver_private {
> struct kobject kobj;
> struct klist klist_devices;
> struct klist_node knode_bus;
> struct module_kobject *mkobj;
> + struct driver_attach_work *attach_work;
> struct device_driver *driver;
I am not arguing, just curious...
Are you trying to shrink sizeof(driver_private) ? The code can be simpler
if you just embedd "struct work_struct attach_work" into driver_private,
and you do not need "struct driver_attach_work" or another ->driver pointer
this way.
Oleg.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
On 09/04, Luis R. Rodriguez wrote: > > From: "Luis R. Rodriguez" > > The new umh kill option has allowed kthreads to receive > kill signals but they are generally accepting all sources > of kill signals And I think this is right, > while the original motivation was to enable > through the OOM from sending the kill. even if the main concern was OOM. > Users can provide a log output and it should be clear on > the trace what probe / driver got the kill signal. Well, if you need a WARN output, perhaps you could just add WARN_ON(fatal_signal_pending()) at the end of load_module() ? Not only kthread_create() can fail if systemd sends SIGKILL. > Although Oleg had rejected a > similar change a while ago And honestly, I still dislike this change. Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] Drivers: scsi: storvsc: Get rid of warning messages
On Thu, Sep 04, 2014 at 10:40:14PM -0700, Christoph Hellwig wrote: > Looks good to me. > > Olaf, Hannes - can I get another review for this (and the older hyperv > scanning patch set)? I agree this looks useful because on a 59753a805499f1ffbca4ac0a24b3dff67bf1 3.17rc2 kernel with 92578ea Drivers: net: hyperv: Cleanup select queue 7e1ea8c Add documentation for HV_STATUS_INVALID_ALIGNMENT. cecd44d BUG: unable to handle kernel paging request at 8801f5bc7cbb (netvsc_select_queue) 9c6196f Drivers: hv: vmbus: Properly protect calls to smp_processor_id() 2590142 Drivers: hv: vmbus: Cleanup hv_post_message() cd97ae9 Drivers: hv: vmbus: Cleanup vmbus_close_internal() ff08f61 Drivers: hv: vmbus: Fix a bug in vmbus_open() 2f8915e Drivers: hv: vmbus: Cleanup vmbus_establish_gpadl() 6940bd6 Drivers: hv: vmbus: Cleanup vmbus_teardown_gpadl() b022b1b Drivers: hv: vmbus: Cleanup vmbus_post_msg() but without this particular patch applied it is possible for a lot of hv_storvsc vmbus_0_5 messages to be produced while running a command like fio --filename /dev/sdg --ioengine=libaio --iodepth=8 --name=go --rw=randwrite --bs=4k --runtime=10m --time_based=1 --direct=1 and repeatedly removing/re-adding/changing the VHDX backing /dev/sdg from the Linux Hyper-V guest: Sep 05 07:09:57 a kernel: scsi 1:0:1:0: scsi scan: INQUIRY result too short (5), using 36 Sep 05 07:09:57 a kernel: scsi 1:0:1:1: scsi scan: INQUIRY result too short (5), using 36 Sep 05 07:09:57 a kernel: sd 1:0:0:0: [sdf] Synchronizing SCSI cache Sep 05 07:09:57 a kernel: hv_storvsc vmbus_0_5: cmd 0x35 scsi status 0x0 srb status 0x20 Sep 05 07:09:58 a kernel: scsi 1:0:0:0: Direct-Access Msft Virtual Disk 1.0 PQ: 0 ANSI: 4 Sep 05 07:09:58 a kernel: sd 1:0:0:0: [sdf] 2097152 512-byte logical blocks: (1.07 GB/1.00 GiB) Sep 05 07:09:58 a kernel: sd 1:0:0:0: Attached scsi generic sg5 type 0 Sep 05 07:09:58 a kernel: scsi 1:0:1:0: scsi scan: INQUIRY result too short (5), using 36 Sep 05 07:09:58 a kernel: sd 1:0:0:0: [sdf] Write Protect is off Sep 05 07:09:58 a kernel: sd 1:0:0:0: [sdf] Mode Sense: 0f 00 00 00 Sep 05 07:09:58 a kernel: sd 1:0:0:0: [sdf] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA Sep 05 07:09:58 a kernel: sdf: unknown partition table Sep 05 07:09:58 a kernel: scsi 1:0:1:1: scsi scan: INQUIRY result too short (5), using 36 Sep 05 07:09:58 a kernel: sd 1:0:0:0: [sdf] Attached SCSI disk Sep 05 07:09:58 a kernel: md: bind Sep 05 07:09:58 a kernel: md126: detected capacity change from 0 to 1069547520 Sep 05 07:09:58 a kernel: md126: unknown partition table Sep 05 07:09:58 a systemd[1]: Starting LVM2 PV scan on device 9:126... Sep 05 07:09:58 a pvscan[788]: 0 logical volume(s) in volume group "PoolDelete" now active Sep 05 07:09:58 a systemd[1]: Started LVM2 PV scan on device 9:126. Sep 05 07:10:01 a systemd[1]: Starting Session 2 of user root. Sep 05 07:10:01 a systemd[1]: Started Session 2 of user root. Sep 05 07:10:01 a CROND[793]: (root) CMD (/usr/lib64/sa/sa1 1 1) Sep 05 07:10:02 a systemd[673]: Time has been changed Sep 05 07:10:02 a systemd[1]: Time has been changed Sep 05 07:10:07 a systemd[1]: Time has been changed Sep 05 07:10:07 a systemd[673]: Time has been changed Sep 05 07:10:12 a systemd[673]: Time has been changed Sep 05 07:10:12 a systemd[1]: Time has been changed Sep 05 07:10:16 a kernel: scsi 1:0:1:0: scsi scan: INQUIRY result too short (5), using 36 Sep 05 07:10:16 a kernel: scsi 1:0:1:1: scsi scan: INQUIRY result too short (5), using 36 Sep 05 07:10:17 a kernel: scsi 1:0:1:0: scsi scan: INQUIRY result too short (5), using 36 Sep 05 07:10:17 a kernel: scsi 1:0:1:1: scsi scan: INQUIRY result too short (5), using 36 Sep 05 07:10:17 a systemd[1]: Time has been changed Sep 05 07:10:17 a systemd[673]: Time has been changed Sep 05 07:10:22 a systemd[673]: Time has been changed Sep 05 07:10:22 a systemd[1]: Time has been changed Sep 05 07:10:27 a systemd[1]: Time has been changed Sep 05 07:10:27 a systemd[673]: Time has been changed Sep 05 07:10:32 a systemd[673]: Time has been changed Sep 05 07:10:32 a systemd[1]: Time has been changed Sep 05 07:10:37 a systemd[1]: Time has been changed Sep 05 07:10:37 a systemd[673]: Time has been changed Sep 05 07:10:42 a systemd[673]: Time has been changed Sep 05 07:10:42 a systemd[1]: Time has been changed Sep 05 07:10:47 a systemd[1]: Time has been changed Sep 05 07:10:47 a systemd[673]: Time has been changed Sep 05 07:10:51 a kernel: scsi 1:0:1:0: scsi scan: INQUIRY result too short (5), using 36 Sep 05 07:10:53 a kernel: hv_storvsc vmbus_0_5: cmd 0x2a scsi status 0x0 srb status 0x20 Sep 05 07:10:53 a kernel: hv_storvsc vmbus_0_5: cmd 0x2a scsi status 0x0 srb status 0x20 Sep 05 07:10:53 a kernel: hv_storvsc vmbus_0_5: cmd 0x2a scsi status 0x0 srb status 0x20 [Message is repeated] Sep 05 07:10:54 a kernel: hv_storvsc vmbus_0_5: cmd 0x2a scsi status 0x0 srb status 0x20 Sep 05 07:10:54 a kernel: hv_storvsc vmbus_0_5: cmd 0x2a scsi status
Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
On Fri, 2014-09-05 at 00:47 -0700, Luis R. Rodriguez wrote: > On Fri, Sep 5, 2014 at 12:19 AM, Tejun Heo wrote: > > On Thu, Sep 04, 2014 at 11:37:24PM -0700, Luis R. Rodriguez wrote: > > ... > >> + /* > >> + * I got SIGKILL, but wait for 60 more seconds for completion > >> + * unless chosen by the OOM killer. This delay is there as a > >> + * workaround for boot failure caused by SIGKILL upon device > >> + * driver initialization timeout. > >> + * > >> + * N.B. this will actually let the thread complete regularly, > >> + * wait_for_completion() will be used eventually, the 60 > >> second > >> + * try here is just to check for the OOM over that time. > >> + */ > >> + WARN_ONCE(!test_thread_flag(TIF_MEMDIE), > >> + "Got SIGKILL but not from OOM, if this issue is on > >> probe use .driver.async_probe\n"); > >> + for (i = 0; i < 60 && !test_thread_flag(TIF_MEMDIE); i++) > >> + if (wait_for_completion_timeout(&done, HZ)) > >> + goto wait_done; > >> + > > > > Ugh... Jesus, this is way too hacky, so now we fail on 90s timeout > > instead of 30? > > Nope! I fell into the same trap and only with tons of patience by part > of Tetsuo with me was I able to grok that the 60 seconds here are not > for increasing the timeout, this is just time spent checking to ensure > that the OOM wasn't the one who triggered the SIGKILL. Even if the > drivers took eons it should be fine now, I tried it :D > > > Why do we even need this with the proposed async > > probing changes? > > Ah -- well without it the way we "find" drivers that need this new > "async feature" is by a bug report and folks saying their system can't > boot, or they say their device doesn't come up. That's all. Tracing > this to systemd and a timeout was one of the most ugliest things ever. > There two insane bug reports you can go check: > > mptsas was the first: > > http://article.gmane.org/gmane.linux.kernel/1669550 > https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1297248 (2) Currently systemd-udevd unconditionally sends SIGKILL upon hardcoded 30 seconds timeout. As a result, finit_module() of mptsas kernel module receives SIGKILL when waiting for error handler thread to be started. Hm. Why is this not a systemd-udevd bug for running around killing stuff when it has no idea whether progress is being made or not? -Mike -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
On Fri, Sep 5, 2014 at 12:19 AM, Tejun Heo wrote: > On Thu, Sep 04, 2014 at 11:37:24PM -0700, Luis R. Rodriguez wrote: > ... >> + /* >> + * I got SIGKILL, but wait for 60 more seconds for completion >> + * unless chosen by the OOM killer. This delay is there as a >> + * workaround for boot failure caused by SIGKILL upon device >> + * driver initialization timeout. >> + * >> + * N.B. this will actually let the thread complete regularly, >> + * wait_for_completion() will be used eventually, the 60 second >> + * try here is just to check for the OOM over that time. >> + */ >> + WARN_ONCE(!test_thread_flag(TIF_MEMDIE), >> + "Got SIGKILL but not from OOM, if this issue is on >> probe use .driver.async_probe\n"); >> + for (i = 0; i < 60 && !test_thread_flag(TIF_MEMDIE); i++) >> + if (wait_for_completion_timeout(&done, HZ)) >> + goto wait_done; >> + > > Ugh... Jesus, this is way too hacky, so now we fail on 90s timeout > instead of 30? Nope! I fell into the same trap and only with tons of patience by part of Tetsuo with me was I able to grok that the 60 seconds here are not for increasing the timeout, this is just time spent checking to ensure that the OOM wasn't the one who triggered the SIGKILL. Even if the drivers took eons it should be fine now, I tried it :D > Why do we even need this with the proposed async > probing changes? Ah -- well without it the way we "find" drivers that need this new "async feature" is by a bug report and folks saying their system can't boot, or they say their device doesn't come up. That's all. Tracing this to systemd and a timeout was one of the most ugliest things ever. There two insane bug reports you can go check: mptsas was the first: http://article.gmane.org/gmane.linux.kernel/1669550 https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1297248 Then cxgb4: https://bugzilla.novell.com/show_bug.cgi?id=877622 I only had Cc'd you on the newest gem pata_marvell : https://bugzilla.kernel.org/show_bug.cgi?id=59581 We can't seriously expect to be doing all this work for every driver. a WARN_ONCE() would enable us to find the drivers that need this new async probe "feature". Luis -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 5/6] mptsas: use async probe
On 09/05/2014 08:37 AM, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez"
>
> Its reported that mptsas can at times take over 30 seconds
> to recognize SCSI storage devices [0], this is done on the
> driver's probe path. Use the the new asynch probe to
> circumvent systemd from killing this driver.
>
> [0] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1276705
>
> Cc: Tetsuo Handa
> Cc: Joseph Salisbury
> Cc: One Thousand Gnomes
> Cc: Tim Gardner
> Cc: Pierre Fersing
> Cc: Andrew Morton
> Cc: Oleg Nesterov
> Cc: Benjamin Poirier
> Cc: Greg Kroah-Hartman
> Cc: Nagalakshmi Nandigama
> Cc: Praveen Krishnamoorthy
> Cc: Sreekanth Reddy
> Cc: Abhijit Mahajan
> Cc: Hariprasad S
> Cc: Santosh Rastapur
> Cc: Casey Leedom
> Cc: mpt-fusionlinux@avagotech.com
> Cc: linux-scsi@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> Cc: net...@vger.kernel.org
> Signed-off-by: Luis R. Rodriguez
> ---
> drivers/message/fusion/mptsas.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
> index 0707fa2..6dfee95 100644
> --- a/drivers/message/fusion/mptsas.c
> +++ b/drivers/message/fusion/mptsas.c
> @@ -5385,6 +5385,7 @@ static struct pci_driver mptsas_driver = {
> .suspend= mptscsih_suspend,
> .resume = mptscsih_resume,
> #endif
> + .driver.async_probe = true,
> };
>
> static int __init
>
This is the wrong appoach.
First of all, the mptsas, mpt2sas, and mpt3sas all share the same
driver layout, so any issue happeing with this driver will most
likely affect the others, too.
Secondly the driver is event-based anyway, so we should be moving
the initialisation to the already existing event handler:
diff --git a/drivers/message/fusion/mptsas.c
b/drivers/message/fusion/mptsas.c
index 0707fa2..6f41e2c 100644
--- a/drivers/message/fusion/mptsas.c
+++ b/drivers/message/fusion/mptsas.c
@@ -5305,7 +5305,7 @@ mptsas_probe(struct pci_dev *pdev, const
struct pci_device_id *id)
/* older firmware doesn't support expander events */
if ((ioc->facts.HeaderVersion >> 8) < 0xE)
ioc->old_sas_discovery_protocal = 1;
- mptsas_scan_sas_topology(ioc);
+ mptsas_queue_rescan(ioc);
mptsas_fw_event_on(ioc);
return 0;
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 3/6] kthread: warn on kill signal if not OOM
On Thu, Sep 04, 2014 at 11:37:24PM -0700, Luis R. Rodriguez wrote: ... > + /* > + * I got SIGKILL, but wait for 60 more seconds for completion > + * unless chosen by the OOM killer. This delay is there as a > + * workaround for boot failure caused by SIGKILL upon device > + * driver initialization timeout. > + * > + * N.B. this will actually let the thread complete regularly, > + * wait_for_completion() will be used eventually, the 60 second > + * try here is just to check for the OOM over that time. > + */ > + WARN_ONCE(!test_thread_flag(TIF_MEMDIE), > + "Got SIGKILL but not from OOM, if this issue is on > probe use .driver.async_probe\n"); > + for (i = 0; i < 60 && !test_thread_flag(TIF_MEMDIE); i++) > + if (wait_for_completion_timeout(&done, HZ)) > + goto wait_done; > + Ugh... Jesus, this is way too hacky, so now we fail on 90s timeout instead of 30? Why do we even need this with the proposed async probing changes? Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 5/6] mptsas: use async probe
On Thu, Sep 04, 2014 at 11:37:26PM -0700, Luis R. Rodriguez wrote: > From: "Luis R. Rodriguez" > > Its reported that mptsas can at times take over 30 seconds > to recognize SCSI storage devices [0], this is done on the > driver's probe path. Use the the new asynch probe to > circumvent systemd from killing this driver. Again, *ANY* SCSI storage controller may. The fact that a specific bug report was filed on mptsas doesn't really mean anything. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
