[PATCH 1/2] target: Don't allow setting WC emulation if device doesn't support

[PATCH 1/2] target: Don't allow setting WC emulation if device doesn't support

[Andy Grover]

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

[PATCH 2/2] target: Fix a comment in emulate_evpd_86

[Andy Grover]

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

Re: [PATCH 1/2] target: Don't allow setting WC emulation if device doesn't support

[Nicholas A. Bellinger]

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

Re: [PATCH 1/2] target: Don't allow setting WC emulation if device doesn't support

[Andy Grover]

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

Re: [PATCH 1/2] target: Don't allow setting WC emulation if device doesn't support

[Nicholas A. Bellinger]

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

Re: [PATCH 1/2] target: Don't allow setting WC emulation if device doesn't support

[Andy Grover]

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