Re: [PATCH 0/5] Feature enhancements for ses module
Thanks, applied to scsi-for-3.20. -- 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 0/5] Feature enhancements for ses module
1: close potential race condition by at enclosure race condition 2,3,4: add enclosure id and device slot, so we can create symlink in /dev/disk/by-slot: # ls -d /dev/disk/by-slot/* /dev/disk/by-slot/enclosure-0x5000ae41fc1310ff-slot0 5: add ability to power on/off device with power_status file in sysfs. Due to the complexity of SES standard, the module is not to replace implement \ all features of sg_ses (sg3_utils). Patch 5 and existing features for device element and array device elements control \ of HDDs. It is helpful to handle some HDD related fields in the kernel, as the \ kernel can generate mapping between a device to the SES device element (or array \ device element): /sys/block/sdc/device/enclosure_deviceXXX/ With patch 5, we can easily power off a running HDD by echo off /sys/block/sdc/device/enclosure_deviceXXX/power_status This is very useful for systems like Cold Storage, where HDDs are being powered \ on/off frequently Dan Williams (4): ses: close potential registration race ses: generate KOBJ_CHANGE on enclosure attach ses: add enclosure logical id ses: add reliable slot attribute Song Liu (1): ses: Add power_status to SES device slot drivers/misc/enclosure.c | 106 + drivers/scsi/ses.c| 148 +++--- include/linux/enclosure.h | 13 +++- 3 files changed, 232 insertions(+), 35 deletions(-) -- 1.8.1 -- 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/5] Feature enhancements for ses module
Hi Doug, I agree what it is difficult to handle all elements, and thus using sg_ses probably makes more sense. However, it helps to handle some HDD related fields in the kernel, as the kernel can generate mapping between a device to the SES device element (or array device element): /sys/block/sdc/device/enclosure_deviceXXX/ With patch 5, we can easily power off a running HDD by echo off /sys/block/sdc/device/enclosure_deviceXXX/power_status This is very useful for systems like Cold Storage, where HDDs are being powered on/off frequently. In current code, ses_set_page2_descriptor already clear reserved field for all elements, and only send non-zero for the device element or array device. Patch 5 also handles reserved field of these two elements in init_device_slot_control: dest_desc[2] = 0xde; dest_desc[3] = 0x3c; So I think we can control fault, locate, active, and power_status of each HDD without issue. Please let me know your suggestion on this. Thanks, Song -Original Message- From: Song Liu Sent: Wednesday, October 22, 2014 8:42 PM To: 'dgilb...@interlog.com'; Jens Axboe; linux-scsi@vger.kernel.org Cc: Hannes Reinecke; Dan Williams; Christoph Hellwig Subject: RE: [PATCH 0/5] Feature enhancements for ses module Hi Doug, The power on/off field together with fault, locate, and active are for HDD (i.e. DeviceSlot and ArrayDeviceSlot). They are not for enclosure or other elements. So we are not dealing with power off duration, etc. here. Thanks, Song -Original Message- From: Douglas Gilbert [mailto:dgilb...@interlog.com] Sent: Wednesday, October 22, 2014 6:17 PM To: Song Liu; Jens Axboe; linux-scsi@vger.kernel.org Cc: Hannes Reinecke; Dan Williams; Christoph Hellwig Subject: Re: [PATCH 0/5] Feature enhancements for ses module On 14-10-23 01:01 AM, Song Liu wrote: Hi Doug, I am not sure whether I fully understand the scenario. Actually patch 5 tries to clear all reserved bits: + dest_desc[0] = 0; + /* only clear byte 1 for ENCLOSURE_COMPONENT_DEVICE */ if + (ecomp-type == ENCLOSURE_COMPONENT_DEVICE) + dest_desc[1] = 0; + dest_desc[2] = 0xde; + dest_desc[3] = 0x3c; Would this work for device that rejects request with 1 in RESERVED areas? That is a pretty asymmetric element type, assuming we are talking about the enclosure control and enclosure status elements (i.e. etc=0xe). My guess would be: dest_desc[0] = 0x80 | (src_desc[0] 40); dest_desc[1] = 0x80 src_desc[1]; dest_desc[2] = (pc_req 6) | pc_delay; dest_desc[3] = 0xff src_desc[3]; or if you have a new power_off_duration: dest_desc[3] = (power_off_duration 2) | (src_desc[3] 0x3); In byte 0 the top bit (SELECT) must be set or the enclosure will ignore any other settings in that element. If the PRDFAIL bit is already set, then that setting will be maintained. SES-3 has a note about clearing DISABLE and SWAP. In byte 1 is if the identifier (LED ?) is active (saying blinking) prior to this power cycle request, then it will stay blinking until the power drops. If the enclosure was really clever it might keep blinking after the power cycle :-) Also notice that the requested power cycle can be cancelled up to the time until power cycle drops to zero. -Original Message- From: Douglas Gilbert [mailto:dgilb...@interlog.com] Sent: Wednesday, October 22, 2014 3:29 PM To: Jens Axboe; Song Liu; linux-scsi@vger.kernel.org Cc: Hannes Reinecke; Dan Williams; Christoph Hellwig Subject: Re: [PATCH 0/5] Feature enhancements for ses module On 14-10-22 09:12 PM, Jens Axboe wrote: On 08/25/2014 11:34 AM, Song Liu wrote: From: Song Liu [mailto:songliubrav...@fb.com] Sent: Monday, August 25, 2014 10:26 AM To: Song Liu Subject: [PATCH 0/5] Feature enhancements for ses module These patches include a few enhancements to ses module: 1: close potential race condition by at enclosure race condition 2,3,4: add enclosure id and device slot, so we can create symlink in /dev/disk/by-slot: # ls -d /dev/disk/by-slot/* /dev/disk/by-slot/enclosure-0x5000ae41fc1310ff-slot0 5: add ability to power on/off device with power_status file in sysfs. Had a rude awakening with sg_ses recently when setting a field in the enclosure control dpage. That is what is being done in point 5: above. The time honoured technique is to read the corresponding enclosure status dpage, find the correct element, twiddle the field of interest, set the SELECT bit and write it back. The idea is maintain any other field settings in that element. And this is what the ses module does. There is at least one SES device out there that rejects the write if there are bits set in RESERVED locations. According to SPC-4 a device may do that. Look at the status (read) and control
Re: [PATCH 0/5] Feature enhancements for ses module
On Wed, Oct 22, 2014 at 01:12:01PM -0600, Jens Axboe wrote: Guys, where are we with these? Seemed like they got reviewed (and updates/fixes posted), then nothing else happened. Would be nice to get these upstream, so we don't have to carry them at FB. Nothign happened, I guess mostly like due to the odd way it was posted, seemingly in reply to something else that wasn't sent in mutt. I'll way for Song and Doug to agree on the reserved field handling and will apply it once all remaining issues there are sorted out. -- 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/5] Feature enhancements for ses module
On 08/25/2014 11:34 AM, Song Liu wrote: From: Song Liu [mailto:songliubrav...@fb.com] Sent: Monday, August 25, 2014 10:26 AM To: Song Liu Subject: [PATCH 0/5] Feature enhancements for ses module These patches include a few enhancements to ses module: 1: close potential race condition by at enclosure race condition 2,3,4: add enclosure id and device slot, so we can create symlink in /dev/disk/by-slot: # ls -d /dev/disk/by-slot/* /dev/disk/by-slot/enclosure-0x5000ae41fc1310ff-slot0 5: add ability to power on/off device with power_status file in sysfs. Dan Williams (4): SES: close potential registration race SES: generate KOBJ_CHANGE on enclosure attach SES: add enclosure logical id SES: add reliable slot attribute Song Liu (1): SES: Add power_status to SES enclosure component Guys, where are we with these? Seemed like they got reviewed (and updates/fixes posted), then nothing else happened. Would be nice to get these upstream, so we don't have to carry them at FB. -- Jens Axboe -- 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/5] Feature enhancements for ses module
On 14-10-22 09:12 PM, Jens Axboe wrote: On 08/25/2014 11:34 AM, Song Liu wrote: From: Song Liu [mailto:songliubrav...@fb.com] Sent: Monday, August 25, 2014 10:26 AM To: Song Liu Subject: [PATCH 0/5] Feature enhancements for ses module These patches include a few enhancements to ses module: 1: close potential race condition by at enclosure race condition 2,3,4: add enclosure id and device slot, so we can create symlink in /dev/disk/by-slot: # ls -d /dev/disk/by-slot/* /dev/disk/by-slot/enclosure-0x5000ae41fc1310ff-slot0 5: add ability to power on/off device with power_status file in sysfs. Had a rude awakening with sg_ses recently when setting a field in the enclosure control dpage. That is what is being done in point 5: above. The time honoured technique is to read the corresponding enclosure status dpage, find the correct element, twiddle the field of interest, set the SELECT bit and write it back. The idea is maintain any other field settings in that element. And this is what the ses module does. There is at least one SES device out there that rejects the write if there are bits set in RESERVED locations. According to SPC-4 a device may do that. Look at the status (read) and control (write) variants of each element type: many times the control variant has less fields. To fix that the read-modify-write cycle needs to be changed to a read-mask-modify-write cycle where the mask is the allowable write mask (there would be one for each element type). This is ugly and may create problems in the future if and when T10 adds a new field in an element. About that time T10 should think about refining the meaning of RESERVED in SES to outlaw SES devices creating this particular time waster. Doug Gilbert Dan Williams (4): SES: close potential registration race SES: generate KOBJ_CHANGE on enclosure attach SES: add enclosure logical id SES: add reliable slot attribute Song Liu (1): SES: Add power_status to SES enclosure component Guys, where are we with these? Seemed like they got reviewed (and updates/fixes posted), then nothing else happened. Would be nice to get these upstream, so we don't have to carry them at FB. -- 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/5] Feature enhancements for ses module
Hi Doug, I am not sure whether I fully understand the scenario. Actually patch 5 tries to clear all reserved bits: + dest_desc[0] = 0; + /* only clear byte 1 for ENCLOSURE_COMPONENT_DEVICE */ + if (ecomp-type == ENCLOSURE_COMPONENT_DEVICE) + dest_desc[1] = 0; + dest_desc[2] = 0xde; + dest_desc[3] = 0x3c; Would this work for device that rejects request with 1 in RESERVED areas? Thanks, Song -Original Message- From: Douglas Gilbert [mailto:dgilb...@interlog.com] Sent: Wednesday, October 22, 2014 3:29 PM To: Jens Axboe; Song Liu; linux-scsi@vger.kernel.org Cc: Hannes Reinecke; Dan Williams; Christoph Hellwig Subject: Re: [PATCH 0/5] Feature enhancements for ses module On 14-10-22 09:12 PM, Jens Axboe wrote: On 08/25/2014 11:34 AM, Song Liu wrote: From: Song Liu [mailto:songliubrav...@fb.com] Sent: Monday, August 25, 2014 10:26 AM To: Song Liu Subject: [PATCH 0/5] Feature enhancements for ses module These patches include a few enhancements to ses module: 1: close potential race condition by at enclosure race condition 2,3,4: add enclosure id and device slot, so we can create symlink in /dev/disk/by-slot: # ls -d /dev/disk/by-slot/* /dev/disk/by-slot/enclosure-0x5000ae41fc1310ff-slot0 5: add ability to power on/off device with power_status file in sysfs. Had a rude awakening with sg_ses recently when setting a field in the enclosure control dpage. That is what is being done in point 5: above. The time honoured technique is to read the corresponding enclosure status dpage, find the correct element, twiddle the field of interest, set the SELECT bit and write it back. The idea is maintain any other field settings in that element. And this is what the ses module does. There is at least one SES device out there that rejects the write if there are bits set in RESERVED locations. According to SPC-4 a device may do that. Look at the status (read) and control (write) variants of each element type: many times the control variant has less fields. To fix that the read-modify-write cycle needs to be changed to a read-mask- modify-write cycle where the mask is the allowable write mask (there would be one for each element type). This is ugly and may create problems in the future if and when T10 adds a new field in an element. About that time T10 should think about refining the meaning of RESERVED in SES to outlaw SES devices creating this particular time waster. Doug Gilbert Dan Williams (4): SES: close potential registration race SES: generate KOBJ_CHANGE on enclosure attach SES: add enclosure logical id SES: add reliable slot attribute Song Liu (1): SES: Add power_status to SES enclosure component Guys, where are we with these? Seemed like they got reviewed (and updates/fixes posted), then nothing else happened. Would be nice to get these upstream, so we don't have to carry them at FB. -- 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/5] Feature enhancements for ses module
On 14-10-23 01:01 AM, Song Liu wrote: Hi Doug, I am not sure whether I fully understand the scenario. Actually patch 5 tries to clear all reserved bits: + dest_desc[0] = 0; + /* only clear byte 1 for ENCLOSURE_COMPONENT_DEVICE */ + if (ecomp-type == ENCLOSURE_COMPONENT_DEVICE) + dest_desc[1] = 0; + dest_desc[2] = 0xde; + dest_desc[3] = 0x3c; Would this work for device that rejects request with 1 in RESERVED areas? That is a pretty asymmetric element type, assuming we are talking about the enclosure control and enclosure status elements (i.e. etc=0xe). My guess would be: dest_desc[0] = 0x80 | (src_desc[0] 40); dest_desc[1] = 0x80 src_desc[1]; dest_desc[2] = (pc_req 6) | pc_delay; dest_desc[3] = 0xff src_desc[3]; or if you have a new power_off_duration: dest_desc[3] = (power_off_duration 2) | (src_desc[3] 0x3); In byte 0 the top bit (SELECT) must be set or the enclosure will ignore any other settings in that element. If the PRDFAIL bit is already set, then that setting will be maintained. SES-3 has a note about clearing DISABLE and SWAP. In byte 1 is if the identifier (LED ?) is active (saying blinking) prior to this power cycle request, then it will stay blinking until the power drops. If the enclosure was really clever it might keep blinking after the power cycle :-) Also notice that the requested power cycle can be cancelled up to the time until power cycle drops to zero. -Original Message- From: Douglas Gilbert [mailto:dgilb...@interlog.com] Sent: Wednesday, October 22, 2014 3:29 PM To: Jens Axboe; Song Liu; linux-scsi@vger.kernel.org Cc: Hannes Reinecke; Dan Williams; Christoph Hellwig Subject: Re: [PATCH 0/5] Feature enhancements for ses module On 14-10-22 09:12 PM, Jens Axboe wrote: On 08/25/2014 11:34 AM, Song Liu wrote: From: Song Liu [mailto:songliubrav...@fb.com] Sent: Monday, August 25, 2014 10:26 AM To: Song Liu Subject: [PATCH 0/5] Feature enhancements for ses module These patches include a few enhancements to ses module: 1: close potential race condition by at enclosure race condition 2,3,4: add enclosure id and device slot, so we can create symlink in /dev/disk/by-slot: # ls -d /dev/disk/by-slot/* /dev/disk/by-slot/enclosure-0x5000ae41fc1310ff-slot0 5: add ability to power on/off device with power_status file in sysfs. Had a rude awakening with sg_ses recently when setting a field in the enclosure control dpage. That is what is being done in point 5: above. The time honoured technique is to read the corresponding enclosure status dpage, find the correct element, twiddle the field of interest, set the SELECT bit and write it back. The idea is maintain any other field settings in that element. And this is what the ses module does. There is at least one SES device out there that rejects the write if there are bits set in RESERVED locations. According to SPC-4 a device may do that. Look at the status (read) and control (write) variants of each element type: many times the control variant has less fields. To fix that the read-modify-write cycle needs to be changed to a read-mask- modify-write cycle where the mask is the allowable write mask (there would be one for each element type). This is ugly and may create problems in the future if and when T10 adds a new field in an element. About that time T10 should think about refining the meaning of RESERVED in SES to outlaw SES devices creating this particular time waster. Doug Gilbert Dan Williams (4): SES: close potential registration race SES: generate KOBJ_CHANGE on enclosure attach SES: add enclosure logical id SES: add reliable slot attribute Song Liu (1): SES: Add power_status to SES enclosure component Guys, where are we with these? Seemed like they got reviewed (and updates/fixes posted), then nothing else happened. Would be nice to get these upstream, so we don't have to carry them at FB. -- 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 -- 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/5] Feature enhancements for ses module
Hi Doug, The power on/off field together with fault, locate, and active are for HDD (i.e. DeviceSlot and ArrayDeviceSlot). They are not for enclosure or other elements. So we are not dealing with power off duration, etc. here. Thanks, Song -Original Message- From: Douglas Gilbert [mailto:dgilb...@interlog.com] Sent: Wednesday, October 22, 2014 6:17 PM To: Song Liu; Jens Axboe; linux-scsi@vger.kernel.org Cc: Hannes Reinecke; Dan Williams; Christoph Hellwig Subject: Re: [PATCH 0/5] Feature enhancements for ses module On 14-10-23 01:01 AM, Song Liu wrote: Hi Doug, I am not sure whether I fully understand the scenario. Actually patch 5 tries to clear all reserved bits: + dest_desc[0] = 0; + /* only clear byte 1 for ENCLOSURE_COMPONENT_DEVICE */ if + (ecomp-type == ENCLOSURE_COMPONENT_DEVICE) + dest_desc[1] = 0; + dest_desc[2] = 0xde; + dest_desc[3] = 0x3c; Would this work for device that rejects request with 1 in RESERVED areas? That is a pretty asymmetric element type, assuming we are talking about the enclosure control and enclosure status elements (i.e. etc=0xe). My guess would be: dest_desc[0] = 0x80 | (src_desc[0] 40); dest_desc[1] = 0x80 src_desc[1]; dest_desc[2] = (pc_req 6) | pc_delay; dest_desc[3] = 0xff src_desc[3]; or if you have a new power_off_duration: dest_desc[3] = (power_off_duration 2) | (src_desc[3] 0x3); In byte 0 the top bit (SELECT) must be set or the enclosure will ignore any other settings in that element. If the PRDFAIL bit is already set, then that setting will be maintained. SES-3 has a note about clearing DISABLE and SWAP. In byte 1 is if the identifier (LED ?) is active (saying blinking) prior to this power cycle request, then it will stay blinking until the power drops. If the enclosure was really clever it might keep blinking after the power cycle :-) Also notice that the requested power cycle can be cancelled up to the time until power cycle drops to zero. -Original Message- From: Douglas Gilbert [mailto:dgilb...@interlog.com] Sent: Wednesday, October 22, 2014 3:29 PM To: Jens Axboe; Song Liu; linux-scsi@vger.kernel.org Cc: Hannes Reinecke; Dan Williams; Christoph Hellwig Subject: Re: [PATCH 0/5] Feature enhancements for ses module On 14-10-22 09:12 PM, Jens Axboe wrote: On 08/25/2014 11:34 AM, Song Liu wrote: From: Song Liu [mailto:songliubrav...@fb.com] Sent: Monday, August 25, 2014 10:26 AM To: Song Liu Subject: [PATCH 0/5] Feature enhancements for ses module These patches include a few enhancements to ses module: 1: close potential race condition by at enclosure race condition 2,3,4: add enclosure id and device slot, so we can create symlink in /dev/disk/by-slot: # ls -d /dev/disk/by-slot/* /dev/disk/by-slot/enclosure-0x5000ae41fc1310ff-slot0 5: add ability to power on/off device with power_status file in sysfs. Had a rude awakening with sg_ses recently when setting a field in the enclosure control dpage. That is what is being done in point 5: above. The time honoured technique is to read the corresponding enclosure status dpage, find the correct element, twiddle the field of interest, set the SELECT bit and write it back. The idea is maintain any other field settings in that element. And this is what the ses module does. There is at least one SES device out there that rejects the write if there are bits set in RESERVED locations. According to SPC-4 a device may do that. Look at the status (read) and control (write) variants of each element type: many times the control variant has less fields. To fix that the read-modify-write cycle needs to be changed to a read-mask- modify-write cycle where the mask is the allowable write mask (there would be one for each element type). This is ugly and may create problems in the future if and when T10 adds a new field in an element. About that time T10 should think about refining the meaning of RESERVED in SES to outlaw SES devices creating this particular time waster. Doug Gilbert Dan Williams (4): SES: close potential registration race SES: generate KOBJ_CHANGE on enclosure attach SES: add enclosure logical id SES: add reliable slot attribute Song Liu (1): SES: Add power_status to SES enclosure component Guys, where are we with these? Seemed like they got reviewed (and updates/fixes posted), then nothing else happened. Would be nice to get these upstream, so we don't have to carry them at FB. -- 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 https://urldefense.proofpoint.com/v1/url?u=http://vger.kernel.org/majo rdomo- info.htmlk=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0Ar=J3jGe56dIPfS5TN6DM 82UYbbeR1j2viaiSJI40tv6lE%3D%0Am
[PATCH 0/5] Feature enhancements for ses module
From: Song Liu [mailto:songliubrav...@fb.com] Sent: Monday, August 25, 2014 10:26 AM To: Song Liu Subject: [PATCH 0/5] Feature enhancements for ses module These patches include a few enhancements to ses module: 1: close potential race condition by at enclosure race condition 2,3,4: add enclosure id and device slot, so we can create symlink in /dev/disk/by-slot: # ls -d /dev/disk/by-slot/* /dev/disk/by-slot/enclosure-0x5000ae41fc1310ff-slot0 5: add ability to power on/off device with power_status file in sysfs. Dan Williams (4): SES: close potential registration race SES: generate KOBJ_CHANGE on enclosure attach SES: add enclosure logical id SES: add reliable slot attribute Song Liu (1): SES: Add power_status to SES enclosure component drivers/misc/enclosure.c | 98 +++ drivers/scsi/ses.c| 145 +++--- include/linux/enclosure.h | 13 - 3 files changed, 220 insertions(+), 36 deletions(-) -- 1.8.1 -- 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
