* [PATCH 1/2] target: Don't allow setting WC emulation if device doesn't support @ 2014-05-14 22:48 Andy Grover 2014-05-14 22:48 ` [PATCH 2/2] target: Fix a comment in emulate_evpd_86 Andy Grover 2014-05-15 0:07 ` [PATCH 1/2] target: Don't allow setting WC emulation if device doesn't support Nicholas A. Bellinger 0 siblings, 2 replies; 6+ messages in thread From: Andy Grover @ 2014-05-14 22:48 UTC (permalink / raw) To: target-devel; +Cc: linux-scsi Just like for pSCSI, if the transport sets get_write_cache, then it is not valid to enable write cache emulation for it. Return an error. see https://bugzilla.redhat.com/show_bug.cgi?id=1082675 Reviewed-by: Chris Leech <cleech@redhat.com> Signed-off-by: Andy Grover <agrover@redhat.com> --- drivers/target/target_core_device.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c index 65001e1..d461ecb 100644 --- a/drivers/target/target_core_device.c +++ b/drivers/target/target_core_device.c @@ -798,10 +798,10 @@ int se_dev_set_emulate_write_cache(struct se_device *dev, int flag) pr_err("emulate_write_cache not supported for pSCSI\n"); return -EINVAL; } - if (dev->transport->get_write_cache) { - pr_warn("emulate_write_cache cannot be changed when underlying" - " HW reports WriteCacheEnabled, ignoring request\n"); - return 0; + if (flag && + dev->transport->get_write_cache) { + pr_err("emulate_write_cache not supported for this device\n"); + return -EINVAL; } dev->dev_attrib.emulate_write_cache = flag; -- 1.9.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] target: Fix a comment in emulate_evpd_86 2014-05-14 22:48 [PATCH 1/2] target: Don't allow setting WC emulation if device doesn't support Andy Grover @ 2014-05-14 22:48 ` Andy Grover 2014-05-15 0:07 ` [PATCH 1/2] target: Don't allow setting WC emulation if device doesn't support Nicholas A. Bellinger 1 sibling, 0 replies; 6+ messages in thread From: Andy Grover @ 2014-05-14 22:48 UTC (permalink / raw) To: target-devel; +Cc: linux-scsi Fix comment to make it clear that check_dev_wce can return true if emulation is in use, but also if it's *not* emulated but the underlying device supports it. Reviewed-by: Chris Leech <cleech@redhat.com> Signed-off-by: Andy Grover <agrover@redhat.com> --- drivers/target/target_core_spc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c index 8653666..bda3a898 100644 --- a/drivers/target/target_core_spc.c +++ b/drivers/target/target_core_spc.c @@ -494,7 +494,7 @@ spc_emulate_evpd_86(struct se_cmd *cmd, unsigned char *buf) /* Set HEADSUP, ORDSUP, SIMPSUP */ buf[5] = 0x07; - /* If WriteCache emulation is enabled, set V_SUP */ + /* If WriteCache supported (or emulated), set V_SUP */ if (spc_check_dev_wce(dev)) buf[6] = 0x01; /* If an LBA map is present set R_SUP */ -- 1.9.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] target: Don't allow setting WC emulation if device doesn't support 2014-05-14 22:48 [PATCH 1/2] target: Don't allow setting WC emulation if device doesn't support Andy Grover 2014-05-14 22:48 ` [PATCH 2/2] target: Fix a comment in emulate_evpd_86 Andy Grover @ 2014-05-15 0:07 ` Nicholas A. Bellinger 2014-05-15 0:22 ` Andy Grover 1 sibling, 1 reply; 6+ messages in thread From: Nicholas A. Bellinger @ 2014-05-15 0:07 UTC (permalink / raw) To: Andy Grover; +Cc: target-devel, linux-scsi On Wed, 2014-05-14 at 15:48 -0700, Andy Grover wrote: > Just like for pSCSI, if the transport sets get_write_cache, then it is > not valid to enable write cache emulation for it. Return an error. > > see https://bugzilla.redhat.com/show_bug.cgi?id=1082675 > > Reviewed-by: Chris Leech <cleech@redhat.com> > Signed-off-by: Andy Grover <agrover@redhat.com> > --- > drivers/target/target_core_device.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c > index 65001e1..d461ecb 100644 > --- a/drivers/target/target_core_device.c > +++ b/drivers/target/target_core_device.c > @@ -798,10 +798,10 @@ int se_dev_set_emulate_write_cache(struct se_device *dev, int flag) > pr_err("emulate_write_cache not supported for pSCSI\n"); > return -EINVAL; > } > - if (dev->transport->get_write_cache) { > - pr_warn("emulate_write_cache cannot be changed when underlying" > - " HW reports WriteCacheEnabled, ignoring request\n"); > - return 0; > + if (flag && > + dev->transport->get_write_cache) { > + pr_err("emulate_write_cache not supported for this device\n"); > + return -EINVAL; > } > > dev->dev_attrib.emulate_write_cache = flag; Allowing the target WCE bit to be disabled when the underlying device has WCE enabled is a recipe for disaster. How is the initiator supposed to know when to flush writes if the target is telling it that WCE is disabled. I'll take change to return -EINVAL here, but the other part is dangerous and wrong. --nab ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] target: Don't allow setting WC emulation if device doesn't support 2014-05-15 0:07 ` [PATCH 1/2] target: Don't allow setting WC emulation if device doesn't support Nicholas A. Bellinger @ 2014-05-15 0:22 ` Andy Grover 2014-05-15 0:57 ` Nicholas A. Bellinger 0 siblings, 1 reply; 6+ messages in thread From: Andy Grover @ 2014-05-15 0:22 UTC (permalink / raw) To: Nicholas A. Bellinger; +Cc: target-devel, linux-scsi On 05/14/2014 05:07 PM, Nicholas A. Bellinger wrote: > On Wed, 2014-05-14 at 15:48 -0700, Andy Grover wrote: >> Just like for pSCSI, if the transport sets get_write_cache, then it is >> not valid to enable write cache emulation for it. Return an error. >> >> see https://bugzilla.redhat.com/show_bug.cgi?id=1082675 >> >> Reviewed-by: Chris Leech <cleech@redhat.com> >> Signed-off-by: Andy Grover <agrover@redhat.com> >> --- >> drivers/target/target_core_device.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c >> index 65001e1..d461ecb 100644 >> --- a/drivers/target/target_core_device.c >> +++ b/drivers/target/target_core_device.c >> @@ -798,10 +798,10 @@ int se_dev_set_emulate_write_cache(struct se_device *dev, int flag) >> pr_err("emulate_write_cache not supported for pSCSI\n"); >> return -EINVAL; >> } >> - if (dev->transport->get_write_cache) { >> - pr_warn("emulate_write_cache cannot be changed when underlying" >> - " HW reports WriteCacheEnabled, ignoring request\n"); >> - return 0; >> + if (flag && >> + dev->transport->get_write_cache) { >> + pr_err("emulate_write_cache not supported for this device\n"); >> + return -EINVAL; >> } >> >> dev->dev_attrib.emulate_write_cache = flag; > > Allowing the target WCE bit to be disabled when the underlying device > has WCE enabled is a recipe for disaster. > > How is the initiator supposed to know when to flush writes if the target > is telling it that WCE is disabled. > > I'll take change to return -EINVAL here, but the other part is dangerous > and wrong. This is for backstores like iblock, where we're actually getting WCE yes/no from the backing block device. This configfs entry is not for WCE yes/no, it is for WCE *emulation* yes/no. So we should allow emulate_write_cache to be set to 0, because we're not emulating the write cache value, we're using the actual value... but we shouldn't allow it to be enabled if we're getting real WCE yes/no values. So I'm still not seeing how this is not the right thing to do, especially since it's identical to the pscsi case immediately above. Regards -- Andy ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] target: Don't allow setting WC emulation if device doesn't support 2014-05-15 0:22 ` Andy Grover @ 2014-05-15 0:57 ` Nicholas A. Bellinger 2014-05-15 2:07 ` Andy Grover 0 siblings, 1 reply; 6+ messages in thread From: Nicholas A. Bellinger @ 2014-05-15 0:57 UTC (permalink / raw) To: Andy Grover; +Cc: target-devel, linux-scsi On Wed, 2014-05-14 at 17:22 -0700, Andy Grover wrote: > On 05/14/2014 05:07 PM, Nicholas A. Bellinger wrote: > > On Wed, 2014-05-14 at 15:48 -0700, Andy Grover wrote: > >> Just like for pSCSI, if the transport sets get_write_cache, then it is > >> not valid to enable write cache emulation for it. Return an error. > >> > >> see https://bugzilla.redhat.com/show_bug.cgi?id=1082675 > >> > >> Reviewed-by: Chris Leech <cleech@redhat.com> > >> Signed-off-by: Andy Grover <agrover@redhat.com> > >> --- > >> drivers/target/target_core_device.c | 8 ++++---- > >> 1 file changed, 4 insertions(+), 4 deletions(-) > >> > >> diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c > >> index 65001e1..d461ecb 100644 > >> --- a/drivers/target/target_core_device.c > >> +++ b/drivers/target/target_core_device.c > >> @@ -798,10 +798,10 @@ int se_dev_set_emulate_write_cache(struct se_device *dev, int flag) > >> pr_err("emulate_write_cache not supported for pSCSI\n"); > >> return -EINVAL; > >> } > >> - if (dev->transport->get_write_cache) { > >> - pr_warn("emulate_write_cache cannot be changed when underlying" > >> - " HW reports WriteCacheEnabled, ignoring request\n"); > >> - return 0; > >> + if (flag && > >> + dev->transport->get_write_cache) { > >> + pr_err("emulate_write_cache not supported for this device\n"); > >> + return -EINVAL; > >> } > >> > >> dev->dev_attrib.emulate_write_cache = flag; > > > > Allowing the target WCE bit to be disabled when the underlying device > > has WCE enabled is a recipe for disaster. > > > > How is the initiator supposed to know when to flush writes if the target > > is telling it that WCE is disabled. > > > > I'll take change to return -EINVAL here, but the other part is dangerous > > and wrong. > > This is for backstores like iblock, where we're actually getting WCE > yes/no from the backing block device. This configfs entry is not for WCE > yes/no, it is for WCE *emulation* yes/no. > Huh..? The only thing that emulate_write_cache controls is the reporting of WCE via the caching mode page + EVPD=0x86 for FILEIO + RD backends. > So we should allow emulate_write_cache to be set to 0, because we're not > emulating the write cache value, we're using the actual value... but we > shouldn't allow it to be enabled if we're getting real WCE yes/no values. There is no point in touching the emulate_write_cache value for IBLOCK. It's already being ignored in spc_check_dev_wce() anyways. > So I'm still not seeing how this is not the right thing to do, > especially since it's identical to the pscsi case immediately above. PSCSI is providing it's own control bits, so the assignment in se_dev_set_emulate_write_cache() is pointless as well. Just get rid of the unnecessary flag check, and make both PSCSI + IBLOCK cases return -EINVAL. --nab ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] target: Don't allow setting WC emulation if device doesn't support 2014-05-15 0:57 ` Nicholas A. Bellinger @ 2014-05-15 2:07 ` Andy Grover 0 siblings, 0 replies; 6+ messages in thread From: Andy Grover @ 2014-05-15 2:07 UTC (permalink / raw) To: Nicholas A. Bellinger; +Cc: target-devel, linux-scsi On 05/14/2014 05:57 PM, Nicholas A. Bellinger wrote: > On Wed, 2014-05-14 at 17:22 -0700, Andy Grover wrote: >> On 05/14/2014 05:07 PM, Nicholas A. Bellinger wrote: >>> On Wed, 2014-05-14 at 15:48 -0700, Andy Grover wrote: >>>> Just like for pSCSI, if the transport sets get_write_cache, then it is >>>> not valid to enable write cache emulation for it. Return an error. >>>> >>>> see https://bugzilla.redhat.com/show_bug.cgi?id=1082675 >>>> >>>> Reviewed-by: Chris Leech <cleech@redhat.com> >>>> Signed-off-by: Andy Grover <agrover@redhat.com> >>>> --- >>>> drivers/target/target_core_device.c | 8 ++++---- >>>> 1 file changed, 4 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c >>>> index 65001e1..d461ecb 100644 >>>> --- a/drivers/target/target_core_device.c >>>> +++ b/drivers/target/target_core_device.c >>>> @@ -798,10 +798,10 @@ int se_dev_set_emulate_write_cache(struct se_device *dev, int flag) >>>> pr_err("emulate_write_cache not supported for pSCSI\n"); >>>> return -EINVAL; >>>> } >>>> - if (dev->transport->get_write_cache) { >>>> - pr_warn("emulate_write_cache cannot be changed when underlying" >>>> - " HW reports WriteCacheEnabled, ignoring request\n"); >>>> - return 0; >>>> + if (flag && >>>> + dev->transport->get_write_cache) { >>>> + pr_err("emulate_write_cache not supported for this device\n"); >>>> + return -EINVAL; >>>> } >>>> >>>> dev->dev_attrib.emulate_write_cache = flag; >>> >>> Allowing the target WCE bit to be disabled when the underlying device >>> has WCE enabled is a recipe for disaster. >>> >>> How is the initiator supposed to know when to flush writes if the target >>> is telling it that WCE is disabled. >>> >>> I'll take change to return -EINVAL here, but the other part is dangerous >>> and wrong. >> >> This is for backstores like iblock, where we're actually getting WCE >> yes/no from the backing block device. This configfs entry is not for WCE >> yes/no, it is for WCE *emulation* yes/no. >> > > Huh..? The only thing that emulate_write_cache controls is the > reporting of WCE via the caching mode page + EVPD=0x86 for FILEIO + RD > backends. > >> So we should allow emulate_write_cache to be set to 0, because we're not >> emulating the write cache value, we're using the actual value... but we >> shouldn't allow it to be enabled if we're getting real WCE yes/no values. > > There is no point in touching the emulate_write_cache value for IBLOCK. > It's already being ignored in spc_check_dev_wce() anyways. Right, but userspace expects that any value it has read from configfs is a valid value to write to that entry when restoring target configuration. So for iblock emulate_write_cache should always read 0, and writing back 0 should succeed. Writing 1 should fail. For this we need to check the flag. Regards -- Andy ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-05-15 2:07 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-05-14 22:48 [PATCH 1/2] target: Don't allow setting WC emulation if device doesn't support Andy Grover 2014-05-14 22:48 ` [PATCH 2/2] target: Fix a comment in emulate_evpd_86 Andy Grover 2014-05-15 0:07 ` [PATCH 1/2] target: Don't allow setting WC emulation if device doesn't support Nicholas A. Bellinger 2014-05-15 0:22 ` Andy Grover 2014-05-15 0:57 ` Nicholas A. Bellinger 2014-05-15 2:07 ` Andy Grover
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).