Re: [PATCH] be2iscsi: Use a more current logging style

2016-08-16 Thread Joe Perches 
On Wed, 2016-08-17 at 09:20 +0530, Jitendra Bhivare wrote:
> > 
> > -Original Message-
> > From: Joe Perches [mailto:j...@perches.com]
> > Sent: Tuesday, August 16, 2016 3:57 PM
> > To: Jitendra Bhivare; Christophe JAILLET; Jayamohan Kallickal; Ketan
> Mukadam
> > 
> > Cc: Bart Van Assche; James E.J. Bottomley; Martin K. Petersen; linux-
> > s...@vger.kernel.org; linux-ker...@vger.kernel.org
> > Subject: Re: [PATCH] be2iscsi: Use a more current logging style
> > 
> > On Tue, 2016-08-16 at 11:32 +0530, Jitendra Bhivare wrote:
> > > 
> > > Thanks Joe for taking this up. It has been pending for long time from
> > > our side.
> > Thanks, not a problem, it took ~10 minutes.
> > 
> > There was a bit of an issue about your reply though.
> > 
> > First there was ~50 k of quoted stuff without any content
> > 
> > > 
> > > [ hundreds and hundreds of quoted lines ]
> > and then this happened:
> > 
> > > 
> > > > 
> > > > diff --git a/drivers/scsi/be2iscsi/be_main.h
> > > b/drivers/scsi/be2iscsi/be_main.h
> > > > 
> > > > 
> > > > index aa9c682..7cce6e3 100644
> > > > --- a/drivers/scsi/be2iscsi/be_main.h
> > > > +++ b/drivers/scsi/be2iscsi/be_main.h
> > > > @@ -1081,15 +1081,19 @@ struct hwi_context_memory {
> > > >  #define BEISCSI_LOG_CONFIG 0x0020  /* CONFIG Code Path */
> > > >  #define BEISCSI_LOG_ISCSI  0x0040  /* SCSI/iSCSI Protocol
> related
> > 
> > > 
> > > Logs */
> > > > 
> > > > 
> > > > 
> > > > -#define __beiscsi_log(phba, level, fmt, ...)
> \
> > 
> > > 
> > > > 
> > > > -   shost_printk(level, phba->shost, fmt, ##__VA_ARGS__)
> > > > -
> > > > -#define beiscsi_log(phba, level, mask, prefix, fmt, ...)   
> > > > \
> > > > +#define beiscsi_printk(level, phba, mask, fmt, ...)
> \
> > 
> > > 
> > > > 
> > > >  do {
> > \
> > > 
> > > > 
> > > > -   uint32_t log_value = phba->attr_log_enable; 
> > > > \
> > > > -   if (((mask) & log_value) || (level[1] <= '3'))  
> > > > \
> > > > -   __beiscsi_log(phba, level, prefix "_%d: " fmt,  
> > > > \
> > > > -     __LINE__, ##__VA_ARGS__); 
> > > > \
> > > > +   if ((mask) & (phba)->attr_log_enable)   
> > > > \
> > > > +   shost_printk(level, phba->shost,
> > > > \
> > > [JB] PCI dev_printk would be more useful with SCSI host_no included by
> > > default in the message.
> > This is a good note that seems simple enough, but I almost missed this.
> > 
> > Given the reply at the top and the _very_ long uncommented quoted block,
> I just
> > 
> > about assumed it was a useless block quote that you didn't bother to
> trim.
> > 
> > 
> > Please make it easier to find your replies and notes by deleting
> irrelevant quoted
> > 
> > stuff.
> > 
> > Also, I think I misread the code.
> > 
> > The original code is <= '3' i.e.: show all KERN_ERR.
> > That is not correct in the new code.
> > 
> > I don't know the code well and don't have a test bed with the hardware.
> > 
> > Is it possible for a beiscsi_ message to be called before
> phba->pcidev is
> > 
> > set to a valid value in beiscsi_hba_alloc?   It appears the code is
> careful to only
> > 
> > use dev_ logging calls before probe.
> [JB] KERN_ERR messages need to be logged irrespective of the masks.
> I understand, that in some places, mask is unnecessarily passed.
> I had made sure to call __beiscsi_log in some places.

I did as well.

> Can we please keep it that way? So beiscsi_err calls dev_err directly or
> is replaced with dev_err.

No worries, I'll respin the series after Christophe's
patches are applied.

> It's safe to assume pcidev will be valid for all beiscsi_log calls.
> Will test your change on my setup before ack'ing.

Don't bother until you get another patchset.

I suggest you fix your email client when sending
replies to me and to lkml.

What I received is very difficult to read due to
the odd line wrapping.

--
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] be2iscsi: Use a more current logging style

2016-08-16 Thread Jitendra Bhivare 
> -Original Message-
> From: Joe Perches [mailto:j...@perches.com]
> Sent: Tuesday, August 16, 2016 3:57 PM
> To: Jitendra Bhivare; Christophe JAILLET; Jayamohan Kallickal; Ketan
Mukadam
> Cc: Bart Van Assche; James E.J. Bottomley; Martin K. Petersen; linux-
> s...@vger.kernel.org; linux-ker...@vger.kernel.org
> Subject: Re: [PATCH] be2iscsi: Use a more current logging style
>
> On Tue, 2016-08-16 at 11:32 +0530, Jitendra Bhivare wrote:
> > Thanks Joe for taking this up. It has been pending for long time from
> > our side.
>
> Thanks, not a problem, it took ~10 minutes.
>
> There was a bit of an issue about your reply though.
>
> First there was ~50 k of quoted stuff without any content
>
> > [ hundreds and hundreds of quoted lines ]
>
> and then this happened:
>
> > > diff --git a/drivers/scsi/be2iscsi/be_main.h
> > b/drivers/scsi/be2iscsi/be_main.h
> > >
> > > index aa9c682..7cce6e3 100644
> > > --- a/drivers/scsi/be2iscsi/be_main.h
> > > +++ b/drivers/scsi/be2iscsi/be_main.h
> > > @@ -1081,15 +1081,19 @@ struct hwi_context_memory {
> > >  #define BEISCSI_LOG_CONFIG   0x0020  /* CONFIG Code Path */
> > >  #define BEISCSI_LOG_ISCSI0x0040  /* SCSI/iSCSI Protocol
related
> > Logs */
> > >
> > >
> > > -#define __beiscsi_log(phba, level, fmt, ...)
\
> > > - shost_printk(level, phba->shost, fmt, ##__VA_ARGS__)
> > > -
> > > -#define beiscsi_log(phba, level, mask, prefix, fmt, ...) \
> > > +#define beiscsi_printk(level, phba, mask, fmt, ...)
\
> > >  do {
>   \
> > > - uint32_t log_value = phba->attr_log_enable; \
> > > - if (((mask) & log_value) || (level[1] <= '3'))  \
> > > - __beiscsi_log(phba, level, prefix "_%d: " fmt,  \
> > > -   __LINE__, ##__VA_ARGS__); \
> > > + if ((mask) & (phba)->attr_log_enable)   \
> > > + shost_printk(level, phba->shost,\
> > [JB] PCI dev_printk would be more useful with SCSI host_no included by
> > default in the message.
>
> This is a good note that seems simple enough, but I almost missed this.
>
> Given the reply at the top and the _very_ long uncommented quoted block,
I just
> about assumed it was a useless block quote that you didn't bother to
trim.
>
> Please make it easier to find your replies and notes by deleting
irrelevant quoted
> stuff.
>
> Also, I think I misread the code.
>
> The original code is <= '3' i.e.: show all KERN_ERR.
> That is not correct in the new code.
>
> I don't know the code well and don't have a test bed with the hardware.
>
> Is it possible for a beiscsi_ message to be called before
phba->pcidev is
> set to a valid value in beiscsi_hba_alloc?   It appears the code is
careful to only
> use dev_ logging calls before probe.
[JB] KERN_ERR messages need to be logged irrespective of the masks.
I understand, that in some places, mask is unnecessarily passed.
I had made sure to call __beiscsi_log in some places.
Can we please keep it that way? So beiscsi_err calls dev_err directly or
is replaced with dev_err.

It's safe to assume pcidev will be valid for all beiscsi_log calls.
Will test your change on my setup before ack'ing.

Actually, we too wanted to get rid of BC_/BM_... line# way and replace
with
ABCD = error identifier.
A 
B 
CD 

But that will be substantial change with some testing requirements. For
now, this looks good.
--
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/2] be2iscsi: Logging neatening

2016-08-16 Thread Joe Perches 
On Wed, 2016-08-17 at 01:19 +, Bart Van Assche wrote:
> On 08/14/16 10:29, Joe Perches wrote:
> > On Sun, 2016-08-14 at 17:09 +, Bart Van Assche wrote:
> > > My primary concern is how to enable and disable log messages from user
> > > space.
[]
> > I think you are looking for a system wide equivalent
> > for the ethtool/netif_ mechanism.
> > 
> > Nothing like that exists currently.
> > 
> > Some code uses a bitmask/and, other code uses a
> > level/comparison.
[]
> As far as I can see all that the ethtool msglevel API implements is a 
> mechanism to query and set the log level from user space. What various 
> SCSI drivers implement is not a log level but a log mask mechanism. How 
> about the following approach to associate a name with each bit in a log 
> mask, to export these names to user space and to make it possible to 
> enable/disable messages per log category:
> * Introduce a variant of pr_debug() that allows to specify a textual
>    representation of the log category (a short string without spaces).
> * Make the log category names available in
>    /sys/kernel/debug/dynamic_debug/...
> * Today dynamic debug allows to enable/disable log messages by
>    specifying the source file name, function name, line number, module
>    name and/or format string. My proposal is to make it also possible to
>    enable/disable log messages based on the log category name.

Many of these logging mechanisms are not just debug
facilities.

Perhaps a dynamic_debug control would be inappropriate.

There have also been various custom scsi log level
facilities like the blogic_msg for the very old
BusLogic blogic_msg.

These functions also sometimes write into some
device-specific buffer.

Perhaps the largest problem, if this is to be scsi only
rather than system wide, is finding out what and how
the various bits in a mask should be used.


--
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 V4 0/2] smartpqi: initial commit of Microsemi smartpqi driver

2016-08-16 Thread Martin K. Petersen 
> "Don" == Don Brace  writes:

Don,

Don> I am thinking that you mean users will need to have some kind of
Don> notification that newer kernels will require that the smartpqi
Don> driver be configured, especially if they are already booting from
Don> the aacraid driver.

One issue is Kconfig. You need to make sure that make oldconfig will
pull in smartpqi if aacraid was previously selected.

The other issue is that the initrd needs to include the smartpqi
module. Hopefully dracut will handle this correctly now. But please test
that it is working correctly when transitioning from an old kernel with
aacraid to a new kernel with smartpqi.

-- 
Martin K. Petersen  Oracle Linux Engineering
--
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/2] be2iscsi: Logging neatening

2016-08-16 Thread Bart Van Assche 
On 08/14/16 10:29, Joe Perches wrote:
> On Sun, 2016-08-14 at 17:09 +, Bart Van Assche wrote:
>> My primary concern is how to enable and disable log messages from user
>> space. Many drivers define their own logging macros and export a bitmask
>> that allows to enable and disable logging messages per category. These
>> bitmask control mechanisms are annoying because figuring out what bit
>> controls which message category requires a search through the driver
>> source code. I'd like to see all these custom logging macros disappear
>> and being replaced by a single mechanism. The "dynamic debug" mechanism
>> e.g. is in my opinion much easier to use than the different custom
>> logging mechanisms.
>
> Dynamic debug doesn't have a bitmask function and
> still requires looking through the code for lines
> and format strings.
>
> I think you are looking for a system wide equivalent
> for the ethtool/netif_ mechanism.
>
> Nothing like that exists currently.
>
> Some code uses a bitmask/and, other code uses a
> level/comparison.
>
> Care to propose something?

Hello Joe,

As far as I can see all that the ethtool msglevel API implements is a 
mechanism to query and set the log level from user space. What various 
SCSI drivers implement is not a log level but a log mask mechanism. How 
about the following approach to associate a name with each bit in a log 
mask, to export these names to user space and to make it possible to 
enable/disable messages per log category:
* Introduce a variant of pr_debug() that allows to specify a textual
   representation of the log category (a short string without spaces).
* Make the log category names available in
   /sys/kernel/debug/dynamic_debug/...
* Today dynamic debug allows to enable/disable log messages by
   specifying the source file name, function name, line number, module
   name and/or format string. My proposal is to make it also possible to
   enable/disable log messages based on the log category name.

Anyway, this is just a proposal. Anyone is welcome to come up with an 
alternative proposal.

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: [PATCH] block: Fix secure erase

2016-08-16 Thread Christoph Hellwig 
On Tue, Aug 16, 2016 at 10:20:25AM +0300, Adrian Hunter wrote:
> On 15/08/16 21:14, Christoph Hellwig wrote:
> > On Mon, Aug 15, 2016 at 11:43:12AM -0500, Shaun Tancheff wrote:
> >> Hmm ... Since REQ_SECURE implied REQ_DISCARD doesn't this
> >> mean that we should include REQ_OP_SECURE_ERASE checking
> >> wherever REQ_OP_DISCARD is being checked now in drivers/scsi/sd.c ?
> >>
> >> (It's only in 3 spots so it's a quickie patch)
> > 
> > SCSI doesn't support secure erase operations.  Only MMC really
> > supports it, plus the usual cargo culting in Xen blkfront that's
> > probably never been tested..
> > 
> 
> I left SCSI out because support does not exist at the moment.
> However there is UFS which is seen as the replacement for eMMC.
> And there is a patch to add support for BLKSECDISCARD:
> 
>   http://marc.info/?l=linux-scsi=146953519016056
> 
> So SCSI will need updating if that is to go in.

That patch is complete crap and if anyone thinks they'd get shit like
that in they are on the same crack that apparently the authors of the
UFS spec are on.

If you want secure discard supported in UFS get a command for into
SBC instead of bypassing the command set.
--
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

[Bug 153241] New: Kernel (unconditionally?) repeatedly attempts to issue SMART commands via ATA pass-through

2016-08-16 Thread bugzilla-daemon 
https://bugzilla.kernel.org/show_bug.cgi?id=153241

Bug ID: 153241
   Summary: Kernel (unconditionally?) repeatedly attempts to issue
SMART commands via ATA pass-through
   Product: IO/Storage
   Version: 2.5
Kernel Version: 4.4.14
  Hardware: All
OS: Linux
  Tree: Mainline
Status: NEW
  Severity: normal
  Priority: P1
 Component: SCSI
  Assignee: linux-scsi@vger.kernel.org
  Reporter: ma...@clara.co.uk
Regression: No

With a recent kernel update (on Lubuntu 16.04 x64-64), I noticed some error
messages in the log on connecting a USB hard drive, for example:

[ 1580.500043] usb 2-1: new high-speed USB device number 4 using ehci-pci
[ 1580.633247] usb 2-1: New USB device found, idVendor=0bc2, idProduct=3300
[ 1580.633255] usb 2-1: New USB device strings: Mfr=1, Product=2,
SerialNumber=3
[ 1580.633260] usb 2-1: Product: Desktop 
[ 1580.633264] usb 2-1: Manufacturer: Seagate 
[ 1580.633268] usb 2-1: SerialNumber: [redacted]
[ 1580.672701] usb-storage 2-1:1.0: USB Mass Storage device detected
[ 1580.674539] scsi host5: usb-storage 2-1:1.0
[ 1580.674639] usbcore: registered new interface driver usb-storage
[ 1580.676522] usbcore: registered new interface driver uas
[ 1581.673205] scsi 5:0:0:0: Direct-Access Seagate  Desktop  0146
PQ: 0 ANSI: 4
[ 1581.676331] sd 5:0:0:0: Attached scsi generic sg2 type 0
[ 1581.677416] sd 5:0:0:0: [sdb] 732566644 4096-byte logical blocks: (3.00
TB/2.73 TiB)
[ 1581.677907] sd 5:0:0:0: [sdb] Write Protect is off
[ 1581.677918] sd 5:0:0:0: [sdb] Mode Sense: 1c 00 00 00
[ 1581.678407] sd 5:0:0:0: [sdb] Write cache: enabled, read cache: enabled,
doesn't support DPO or FUA
[ 1581.690549]  sdb: sdb1
[ 1581.692636] sd 5:0:0:0: [sdb] Attached SCSI disk
[ 1581.846416] sd 5:0:0:0: [sdb] tag#0 FAILED Result: hostbyte=DID_ERROR
driverbyte=DRIVER_SENSE
[ 1581.846426] sd 5:0:0:0: [sdb] tag#0 Sense Key : Hardware Error [current]
[descriptor] 
[ 1581.846432] sd 5:0:0:0: [sdb] tag#0 Add. Sense: No additional sense
information
[ 1581.846439] sd 5:0:0:0: [sdb] tag#0 CDB: ATA command pass through(16) 85 06
20 00 00 00 00 00 00 00 00 00 00 00 e5 00
[ 1581.929398] sd 5:0:0:0: [sdb] tag#0 FAILED Result: hostbyte=DID_ERROR
driverbyte=DRIVER_SENSE
[ 1581.929403] sd 5:0:0:0: [sdb] tag#0 Sense Key : Hardware Error [current]
[descriptor] 
[ 1581.929405] sd 5:0:0:0: [sdb] tag#0 Add. Sense: No additional sense
information
[ 1581.929409] sd 5:0:0:0: [sdb] tag#0 CDB: ATA command pass through(12)/Blank
a1 06 20 da 00 00 4f c2 00 b0 00 00

The ATA pass-through commands are attempted every ten minutes, so the tag#0...
lines repeat in the kernel log. The commands are something to do with SMART.

It seems other people are seeing this problem too:
"kernel-4.6.3-300 shows false warnings about USB hard disks."
https://bugzilla.redhat.com/show_bug.cgi?id=1351305

"Worrisome USB disk messages in 4.6.0-1, but not 4.5.0-2"
https://lists.debian.org/debian-user/2016/07/msg00988.html


I bisected between 4.4.13 and 4.4.14 with this result:

$ git bisect bad
0dec8c0d67c64401d97122e4eba347ccc5850622 is the first bad commit
commit 0dec8c0d67c64401d97122e4eba347ccc5850622
Author: James Bottomley 
Date:   Fri May 13 12:04:06 2016 -0700

scsi_lib: correctly retry failed zero length REQ_TYPE_FS commands

commit a621bac3044ed6f7ec5fa0326491b2d4838bfa93 upstream.

When SCSI was written, all commands coming from the filesystem
(REQ_TYPE_FS commands) had data.  This meant that our signal for needing
to complete the command was the number of bytes completed being equal to
the number of bytes in the request.  Unfortunately, with the advent of
flush barriers, we can now get zero length REQ_TYPE_FS commands, which
confuse this logic because they satisfy the condition every time.  This
means they never get retried even for retryable conditions, like UNIT
ATTENTION because we complete them early assuming they're done.  Fix
this by special casing the early completion condition to recognise zero
length commands with errors and let them drop through to the retry code.

Reported-by: Sebastian Parschauer 
Signed-off-by: James E.J. Bottomley 
Tested-by: Jack Wang 
Signed-off-by: Martin K. Petersen 
Signed-off-by: Greg Kroah-Hartman 


>From that commit description, it sounds like it may not be the real/root cause
of the problem though? Just that now, commands which might have been silently
dropped/ignored before, no longer are.


So, questions...

Does the kernel now unconditionally try to issue ATA pass-through commands
(opcodes 0x85 and 0xA1)? That could be problematic for several reasons:

 - The FAILED log messages are likely to make the

Re: [GIT PULL] [PATCH v4 00/26] Delete CURRENT_TIME and CURRENT_TIME_SEC macros

2016-08-16 Thread Greg KH 
On Tue, Aug 16, 2016 at 11:18:52AM -0700, Deepa Dinamani wrote:
> Thank you for the suggestion.
> 
> > Who are you execting to pull this huge patch series?
> 
> The last pull request was addressed to Al as per Arnd's suggestion.
> I'm not completely sure who should it be addressed to.
> 
> > Why not just introduce the new api call, wait for that to be merged, and
> > then push the individual patches through the different subsystems?
> > After half of those get ignored, then provide a single set of patches
> > that can go through Andrew or my trees.
> 
> Arnd and I tried to do this a few ways.
> 
> We can try to introduce the api first like you suggest.
> 
> There are a few Acks already on the patches.
> And, patches 2-5 also need to be merged through some common tree like
> yours or Andrew's as you suggest.
> 
> So, if everyone is ok, I could do the following:
> 
> 1. Post patches 1-5 for rc-2.

-rc2 is already released, and we aren't adding new apis this late in the
release cycle, sorry.

> 2. Post all other patches to respective maintainers after rc-2
> 3. Then after patches get ignored or merged, post remaining as a
> series for you or Andrew to pick up.

The apis need to be aimed for 4.9-rc1, it's too late for 4.8, sorry.

greg k-h
--
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: [GIT PULL] [PATCH v4 00/26] Delete CURRENT_TIME and CURRENT_TIME_SEC macros

2016-08-16 Thread Deepa Dinamani 
Thank you for the suggestion.

> Who are you execting to pull this huge patch series?

The last pull request was addressed to Al as per Arnd's suggestion.
I'm not completely sure who should it be addressed to.

> Why not just introduce the new api call, wait for that to be merged, and
> then push the individual patches through the different subsystems?
> After half of those get ignored, then provide a single set of patches
> that can go through Andrew or my trees.

Arnd and I tried to do this a few ways.

We can try to introduce the api first like you suggest.

There are a few Acks already on the patches.
And, patches 2-5 also need to be merged through some common tree like
yours or Andrew's as you suggest.

So, if everyone is ok, I could do the following:

1. Post patches 1-5 for rc-2.
2. Post all other patches to respective maintainers after rc-2
3. Then after patches get ignored or merged, post remaining as a
series for you or Andrew to pick up.

-Deepa
--
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: bnx2i: convert to kworker

2016-08-16 Thread Chad Dupuis 

On Fri, 12 Aug 2016, 9:24pm -, Martin K. Petersen wrote:

> > "Sebastian" == Sebastian Andrzej Siewior  writes:
> 
> Sebastian> On 2016-07-04 19:40:37 [+0200], To linux-scsi@vger.kernel.org 
> wrote:
> >> The driver creates its own per-CPU threads which are updated based on
> >> CPU hotplug events. It is also possible to use kworkers and remove
> >> some of the infrastructure get the same job done while saving a few
> >> lines of code.
> 
> Sebastian> ping
> 
> People generally don't rummage through their inboxes looking for old
> stuff to review. If nobody has responded to your patch within a week or
> two it's best to resubmit and get the proposed changes back on their
> screens.
> 

Yes, please repost. 
--
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/5] nvme-fabrics: Add FC transport FC-NVME definitions

2016-08-16 Thread James Smart 

yes - planned to start that today

-- james


On 8/16/2016 3:01 AM, Sagi Grimberg wrote:



Looks fine,

Reviewed-by: Christoph Hellwig 


Hey James,

Can you collect review tags, address CR comments and resend the series?
I'd like to stage these for 0-day testing and try to get it into 4.9.

Thanks,
Sagi.


--
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 1/5] multipath-tools: document how to add devices that need special flags in the SCSI subsystem

2016-08-16 Thread Xose Vazquez Perez 
Full flags list at include/scsi/scsi_devinfo.h

Arrays controllers included currently:

 {"3PARdata", "VV", NULL, BLIST_REPORTLUN2},

 {"DEC", "HSG80", NULL, BLIST_REPORTLUN2 | BLIST_NOSTARTONADD},
 {"HP", "A6189A", NULL, BLIST_SPARSELUN | BLIST_LARGELUN},
 {"COMPAQ", "MSA1000", NULL, BLIST_SPARSELUN | BLIST_NOSTARTONADD},
 {"COMPAQ", "MSA1000 VOLUME", NULL, BLIST_SPARSELUN | BLIST_NOSTARTONADD},
 {"COMPAQ", "HSV110", NULL, BLIST_REPORTLUN2 | BLIST_NOSTARTONADD},
 {"HP", "HSV100", NULL, BLIST_REPORTLUN2 | BLIST_NOSTARTONADD},
 {"COMPAQ", "LOGICAL VOLUME", NULL, BLIST_FORCELUN | BLIST_MAX_512},

 {"DDN", "SAN DataDirector", "*", BLIST_SPARSELUN},

 {"DGC", "RAID", NULL, BLIST_SPARSELUN},
 {"DGC", "DISK", NULL, BLIST_SPARSELUN},
<-- mising VDISK ??
 {"EMC", "SYMMETRIX", NULL, BLIST_SPARSELUN | BLIST_LARGELUN | BLIST_FORCELUN},
 {"EMC",  "Invista", "*", BLIST_SPARSELUN | BLIST_LARGELUN},

 {"FSC", "CentricStor", "*", BLIST_SPARSELUN | BLIST_LARGELUN},

 {"HITACHI", "OPEN-", "*", BLIST_REPORTLUN2},
 {"HP", "OPEN-", "*", BLIST_REPORTLUN2},

 {"HITACHI", "DF400", "*", BLIST_REPORTLUN2},
 {"HITACHI", "DF500", "*", BLIST_REPORTLUN2},
 {"HP", "DF400", "*", BLIST_SPARSELUN | BLIST_LARGELUN},
 {"HP", "DF500", "*", BLIST_SPARSELUN | BLIST_LARGELUN},
 {"HP", "DF600", "*", BLIST_SPARSELUN | BLIST_LARGELUN},
<-- bug, also apply: 627511e3e

 {"IBM", "ProFibre 4000R", "*", BLIST_SPARSELUN | BLIST_LARGELUN},

 {"NETAPP", "LUN C-Mode", NULL, BLIST_SYNC_ALUA},   
<-- ontap

 {"NETAPP", "INF-01-00", NULL, BLIST_SYNC_ALUA},
<-- rdac!!

 {"SGI", "TP9100", "*", BLIST_REPORTLUN2},

 {"Intel", "Multi-Flex", NULL, BLIST_NO_RSOC},

 {"SGI", "Universal Xport", "*", BLIST_NO_ULD_ATTACH},
 {"IBM", "Universal Xport", "*", BLIST_NO_ULD_ATTACH},
 {"SUN", "Universal Xport", "*", BLIST_NO_ULD_ATTACH},
 {"DELL", "Universal Xport", "*", BLIST_NO_ULD_ATTACH},

Cc: Christophe Varoqui 
Cc: James E.J. Bottomley 
Cc: Martin K. Petersen 
Cc: SCSI ML 
Cc: device-mapper development 
Signed-off-by: Xose Vazquez Perez 
---
 libmultipath/hwtable.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/libmultipath/hwtable.c b/libmultipath/hwtable.c
index 1bc6f2c..4e1740a 100644
--- a/libmultipath/hwtable.c
+++ b/libmultipath/hwtable.c
@@ -22,6 +22,9 @@
  *
  * Devices with a proprietary handler must also be included in
  * the kernel side. Currently at drivers/scsi/scsi_dh.c
+ *
+ * Moreover, if a device needs a special treatment by the SCSI
+ * subsystem it should be included in drivers/scsi/scsi_devinfo.c
  */
 static struct hwentry default_hw[] = {
/*
-- 
2.7.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

[PATCH] scsi: introduce a quirk for false cache reporting

2016-08-16 Thread Oliver Neukum 
Some SATA to USB bridges fail to cooperate with some
drives resulting in no cache being present being reported
to the host. That causes the host to skip sending
a command to synchronize caches. That causes data loss
when the drive is powered down.

Signed-off-by: Oliver Neukum 
---
 Documentation/kernel-parameters.txt | 2 ++
 drivers/usb/storage/scsiglue.c  | 8 
 drivers/usb/storage/unusual_devs.h  | 7 +++
 drivers/usb/storage/usb.c   | 6 +-
 include/linux/usb_usual.h   | 2 ++
 5 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/Documentation/kernel-parameters.txt 
b/Documentation/kernel-parameters.txt
index 82b42c9..c8c682e 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -4182,6 +4182,8 @@ bytes respectively. Such letter suffixes can also be 
entirely omitted.
u = IGNORE_UAS (don't bind to the uas driver);
w = NO_WP_DETECT (don't test whether the
medium is write-protected).
+   y = ALWAYS_SYNC (issue a SYNCHRONIZE_CACHE
+   even if the device claims no cache)
Example: quirks=0419:aaf5:rl,0421:0433:rc
 
user_debug= [KNL,ARM]
diff --git a/drivers/usb/storage/scsiglue.c b/drivers/usb/storage/scsiglue.c
index 33eb923..8cd2926 100644
--- a/drivers/usb/storage/scsiglue.c
+++ b/drivers/usb/storage/scsiglue.c
@@ -296,6 +296,14 @@ static int slave_configure(struct scsi_device *sdev)
if (us->fflags & US_FL_BROKEN_FUA)
sdev->broken_fua = 1;
 
+   /* Some even totally fail to indicate a cache */
+   if (us->fflags & US_FL_ALWAYS_SYNC) {
+   /* don't read caching information */
+   sdev->skip_ms_page_8 = 1;
+   sdev->skip_ms_page_3f = 1;
+   /* assume sync is needed */
+   sdev->wce_default_on = 1;
+   }
} else {
 
/*
diff --git a/drivers/usb/storage/unusual_devs.h 
b/drivers/usb/storage/unusual_devs.h
index aa35392..af3c7ee 100644
--- a/drivers/usb/storage/unusual_devs.h
+++ b/drivers/usb/storage/unusual_devs.h
@@ -338,6 +338,13 @@ UNUSUAL_DEV(  0x046b, 0xff40, 0x0100, 0x0100,
USB_SC_DEVICE, USB_PR_DEVICE, NULL,
US_FL_NO_WP_DETECT),
 
+/* Reported by Egbert Eich  */
+UNUSUAL_DEV(  0x0480, 0xd010, 0x0100, 0x,
+   "Toshiba",
+   "External USB 3.0",
+   USB_SC_DEVICE, USB_PR_DEVICE, NULL,
+   US_FL_ALWAYS_SYNC),
+
 /* Patch submitted by Philipp Friedrich  */
 UNUSUAL_DEV(  0x0482, 0x0100, 0x0100, 0x0100,
"Kyocera",
diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c
index ef2d8cd..19255f1 100644
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -498,7 +498,8 @@ void usb_stor_adjust_quirks(struct usb_device *udev, 
unsigned long *fflags)
US_FL_NO_READ_DISC_INFO | US_FL_NO_READ_CAPACITY_16 |
US_FL_INITIAL_READ10 | US_FL_WRITE_CACHE |
US_FL_NO_ATA_1X | US_FL_NO_REPORT_OPCODES |
-   US_FL_MAX_SECTORS_240 | US_FL_NO_REPORT_LUNS);
+   US_FL_MAX_SECTORS_240 | US_FL_NO_REPORT_LUNS |
+   US_FL_ALWAYS_SYNC);
 
p = quirks;
while (*p) {
@@ -581,6 +582,9 @@ void usb_stor_adjust_quirks(struct usb_device *udev, 
unsigned long *fflags)
case 'w':
f |= US_FL_NO_WP_DETECT;
break;
+   case 'y':
+   f |= US_FL_ALWAYS_SYNC;
+   break;
/* Ignore unrecognized flag characters */
}
}
diff --git a/include/linux/usb_usual.h b/include/linux/usb_usual.h
index 245f57d..0aae1b2 100644
--- a/include/linux/usb_usual.h
+++ b/include/linux/usb_usual.h
@@ -81,6 +81,8 @@
/* Sets max_sectors to 240 */   \
US_FLAG(NO_REPORT_LUNS, 0x1000) \
/* Cannot handle REPORT_LUNS */ \
+   US_FLAG(ALWAYS_SYNC, 0x2000)\
+   /* lies about caching, so always sync */\
 
 #define US_FLAG(name, value)   US_FL_##name = value ,
 enum { US_DO_ALL_FLAGS };
-- 
2.1.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: introduce a quirk for false cache reporting

2016-08-16 Thread Oliver Neukum 
On Tue, 2016-08-16 at 00:44 -0400, Martin K. Petersen wrote:
> > "Oliver" == Oliver Neukum  writes:
> 
> Oliver,
> 
> Oliver> wce_default_on controls the default if the device provides no
> Oliver> indication. The problem here is that the indication the device
> Oliver> provides must be overridden, as they are false.
> 
> Can't you just blacklist the mode select on the device in question?
> 
> Something like:
> 
>   if (us->fflags & US_FL_ALWAYS_SYNC) {
>   sdev->skip_ms_page_3f = 1;
>   sdev->skip_ms_page_8 = 1;
>   sdev->wce_default_on = 1;
>   }
> 
> ?

Hi,

this looks like a workable option. I am preparing a patch.

Regards
Oliver




--
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] be2iscsi: Use a more current logging style

2016-08-16 Thread Joe Perches 
On Tue, 2016-08-16 at 11:32 +0530, Jitendra Bhivare wrote:
> Thanks Joe for taking this up. It has been pending for long time from our
> side.

Thanks, not a problem, it took ~10 minutes.

There was a bit of an issue about your reply though.

First there was ~50 k of quoted stuff without any content

> [ hundreds and hundreds of quoted lines ]

and then this happened:

> > diff --git a/drivers/scsi/be2iscsi/be_main.h
> b/drivers/scsi/be2iscsi/be_main.h
> > 
> > index aa9c682..7cce6e3 100644
> > --- a/drivers/scsi/be2iscsi/be_main.h
> > +++ b/drivers/scsi/be2iscsi/be_main.h
> > @@ -1081,15 +1081,19 @@ struct hwi_context_memory {
> >  #define BEISCSI_LOG_CONFIG 0x0020  /* CONFIG Code Path */
> >  #define BEISCSI_LOG_ISCSI  0x0040  /* SCSI/iSCSI Protocol related
> Logs */
> > 
> > 
> > -#define __beiscsi_log(phba, level, fmt, ...)   
> > \
> > -   shost_printk(level, phba->shost, fmt, ##__VA_ARGS__)
> > -
> > -#define beiscsi_log(phba, level, mask, prefix, fmt, ...)   \
> > +#define beiscsi_printk(level, phba, mask, fmt, ...)
> > \
> >  do {   
> > \
> > -   uint32_t log_value = phba->attr_log_enable; \
> > -   if (((mask) & log_value) || (level[1] <= '3'))  \
> > -   __beiscsi_log(phba, level, prefix "_%d: " fmt,  \
> > -     __LINE__, ##__VA_ARGS__); \
> > +   if ((mask) & (phba)->attr_log_enable)   \
> > +   shost_printk(level, phba->shost,\
> [JB] PCI dev_printk would be more useful with SCSI host_no included by
> default in the message.

This is a good note that seems simple enough, but I almost
missed this.

Given the reply at the top and the _very_ long uncommented
quoted block, I just about assumed it was a useless block
quote that you didn't bother to trim.

Please make it easier to find your replies and notes by
deleting irrelevant quoted stuff.

Also, I think I misread the code.

The original code is <= '3' i.e.: show all KERN_ERR.
That is not correct in the new code.

I don't know the code well and don't have a test bed with
the hardware.

Is it possible for a beiscsi_ message to be called
before phba->pcidev is set to a valid value in
beiscsi_hba_alloc?   It appears the code is careful to
only use dev_ logging calls before probe.

--
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/5] nvme-fabrics: Add FC transport FC-NVME definitions

2016-08-16 Thread Sagi Grimberg 



Looks fine,

Reviewed-by: Christoph Hellwig 


Hey James,

Can you collect review tags, address CR comments and resend the series?
I'd like to stage these for 0-day testing and try to get it into 4.9.

Thanks,
Sagi.
--
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] block: Fix secure erase

2016-08-16 Thread Adrian Hunter 
On 15/08/16 21:14, Christoph Hellwig wrote:
> On Mon, Aug 15, 2016 at 11:43:12AM -0500, Shaun Tancheff wrote:
>> Hmm ... Since REQ_SECURE implied REQ_DISCARD doesn't this
>> mean that we should include REQ_OP_SECURE_ERASE checking
>> wherever REQ_OP_DISCARD is being checked now in drivers/scsi/sd.c ?
>>
>> (It's only in 3 spots so it's a quickie patch)
> 
> SCSI doesn't support secure erase operations.  Only MMC really
> supports it, plus the usual cargo culting in Xen blkfront that's
> probably never been tested..
> 

I left SCSI out because support does not exist at the moment.
However there is UFS which is seen as the replacement for eMMC.
And there is a patch to add support for BLKSECDISCARD:

http://marc.info/?l=linux-scsi=146953519016056

So SCSI will need updating if that is to go in.

--
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/2] be2iscsi: Fix error return code

2016-08-16 Thread Jitendra Bhivare 
> -Original Message-
> From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
> ow...@vger.kernel.org] On Behalf Of Christophe JAILLET
> Sent: Friday, August 12, 2016 3:33 PM
> To: jayamohan.kallic...@avagotech.com; j...@linux.vnet.ibm.com;
> ketan.muka...@avagotech.com; sony.j...@avagotech.com;
> martin.peter...@oracle.com
> Cc: linux-scsi@vger.kernel.org; linux-ker...@vger.kernel.org; kernel-
> janit...@vger.kernel.org; Christophe JAILLET
> Subject: [PATCH 1/2] be2iscsi: Fix error return code
>
> We know that 'ret' is not an error code because it has been tested a few
> lines
> above.
> So, if one of these function fails, 0 will be returned instead of an error
> code.
> Return -ENOMEM instead.
>
> Signed-off-by: Christophe JAILLET 
> ---
>  drivers/scsi/be2iscsi/be_main.c | 11 +--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/be2iscsi/be_main.c
> b/drivers/scsi/be2iscsi/be_main.c
> index f05e7737107d..89ae6390b697 100644
> --- a/drivers/scsi/be2iscsi/be_main.c
> +++ b/drivers/scsi/be2iscsi/be_main.c
> @@ -3286,8 +3286,10 @@ static int beiscsi_create_eqs(struct beiscsi_hba
> *phba,
>   eq_vaddress = pci_alloc_consistent(phba->pcidev,
>num_eq_pages *
> PAGE_SIZE,
>);
> - if (!eq_vaddress)
> + if (!eq_vaddress) {
> + ret = -ENOMEM;
>   goto create_eq_error;
> + }
>
>   mem->va = eq_vaddress;
>   ret = be_fill_queue(eq, phba->params.num_eq_entries, @@ -
> 3349,8 +3351,11 @@ static int beiscsi_create_cqs(struct beiscsi_hba *phba,
>   cq_vaddress = pci_alloc_consistent(phba->pcidev,
>num_cq_pages *
> PAGE_SIZE,
>);
> - if (!cq_vaddress)
> + if (!cq_vaddress) {
> + ret = -ENOMEM;
>   goto create_cq_error;
> + }
> +
>   ret = be_fill_queue(cq, phba->params.num_cq_entries,
>   sizeof(struct sol_cqe), cq_vaddress);
>   if (ret) {
> @@ -5635,6 +5640,7 @@ static int beiscsi_dev_probe(struct pci_dev *pcidev,
>   if (!phba) {
>   dev_err(>dev,
>   "beiscsi_dev_probe - Failed in beiscsi_hba_alloc\n");
> + ret = -ENOMEM;
>   goto disable_pci;
>   }
>
> @@ -5754,6 +5760,7 @@ static int beiscsi_dev_probe(struct pci_dev *pcidev,
>   beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_INIT,
>   "BM_%d : beiscsi_dev_probe-"
>   "Failed to allocate work queue\n");
> + ret = -ENOMEM;
>   goto free_twq;
>   }
>

[JB] This still has to be fixed, right?
--
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] be2iscsi: Use a more current logging style

2016-08-16 Thread Jitendra Bhivare 
Thanks Joe for taking this up. It has been pending for long time from our
side.

> -Original Message-
> From: linux-scsi-ow...@vger.kernel.org [mailto:linux-scsi-
> ow...@vger.kernel.org] On Behalf Of Joe Perches
> Sent: Monday, August 15, 2016 12:26 AM
> To: Christophe JAILLET; Jayamohan Kallickal; Ketan Mukadam
> Cc: Bart Van Assche; James E.J. Bottomley; Martin K. Petersen; linux-
> s...@vger.kernel.org; linux-ker...@vger.kernel.org
> Subject: [PATCH] be2iscsi: Use a more current logging style
>
> shost_printk macros are converted to beiscsi_ for  a
> more current logging style.
>
> Consolidate the masked tests into a beiscsi_printk macro and
> use that to call shost_printk.
>
> Convert the __beiscsi_log macros to use shost_print directly.
>
> Miscellanea:
>
> o Remove the file prefix identifier and use kbasename(__FILE__)
>   and stringify(__LINE__) to reduce code size in beiscsi_printk
> o Realign arguments
>
> Signed-off-by: Joe Perches 
> ---
>  drivers/scsi/be2iscsi/be_cmds.c  | 142 
>  drivers/scsi/be2iscsi/be_iscsi.c | 223 ++--
>  drivers/scsi/be2iscsi/be_main.c  | 733
+++
>  drivers/scsi/be2iscsi/be_main.h  |  20 +-
>  drivers/scsi/be2iscsi/be_mgmt.c  | 270 +++---
>  5 files changed, 668 insertions(+), 720 deletions(-)
>
> diff --git a/drivers/scsi/be2iscsi/be_cmds.c
b/drivers/scsi/be2iscsi/be_cmds.c
> index a51bc0e..574d2f4 100644
> --- a/drivers/scsi/be2iscsi/be_cmds.c
> +++ b/drivers/scsi/be2iscsi/be_cmds.c
> @@ -95,8 +95,8 @@ int be_chk_reset_complete(struct beiscsi_hba *phba)
>   }
>
>   if ((status & 0x8000) || (!num_loop)) {
> - beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_INIT,
> - "BC", "Failed in be_chk_reset_complete status
=
> 0x%x\n",
> + beiscsi_err(phba, BEISCSI_LOG_INIT,
> + "Failed in be_chk_reset_complete status =
0x%x\n",
>   status);
>   return -EIO;
>   }
> @@ -135,9 +135,9 @@ struct be_mcc_wrb *alloc_mcc_wrb(struct beiscsi_hba
> *phba,
>
>   spin_lock_bh(>ctrl.mcc_lock);
>   if (mccq->used == mccq->len) {
> - beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_INIT |
> + beiscsi_err(phba, BEISCSI_LOG_INIT |
>   BEISCSI_LOG_CONFIG | BEISCSI_LOG_MBOX,
> - "BC", "MCC queue full: WRB used %u tag avail
%u\n",
> + "MCC queue full: WRB used %u tag avail %u\n",
>   mccq->used, phba->ctrl.mcc_tag_available);
>   goto alloc_failed;
>   }
> @@ -147,9 +147,9 @@ struct be_mcc_wrb *alloc_mcc_wrb(struct beiscsi_hba
> *phba,
>
>   tag = phba->ctrl.mcc_tag[phba->ctrl.mcc_alloc_index];
>   if (!tag) {
> - beiscsi_log(phba, KERN_ERR, BEISCSI_LOG_INIT |
> + beiscsi_err(phba, BEISCSI_LOG_INIT |
>   BEISCSI_LOG_CONFIG | BEISCSI_LOG_MBOX,
> - "BC", "MCC tag 0 allocated: tag avail %u alloc
index
> %u\n",
> + "MCC tag 0 allocated: tag avail %u alloc index
%u\n",
>   phba->ctrl.mcc_tag_available,
>   phba->ctrl.mcc_alloc_index);
>   goto alloc_failed;
> @@ -263,10 +263,10 @@ int beiscsi_mccq_compl_wait(struct beiscsi_hba
> *phba,
>   set_bit(MCC_TAG_STATE_TIMEOUT,
>   >ctrl.ptag_state[tag].tag_state);
>
> - beiscsi_log(phba, KERN_ERR,
> + beiscsi_err(phba,
>   BEISCSI_LOG_INIT | BEISCSI_LOG_EH |
>   BEISCSI_LOG_CONFIG,
> - "BC", "MBX Cmd Completion timed out\n");
> + "MBX Cmd Completion timed out\n");
>   return -EBUSY;
>   }
>
> @@ -289,22 +289,22 @@ int beiscsi_mccq_compl_wait(struct beiscsi_hba
> *phba,
>   }
>
>   if (status || addl_status) {
> - beiscsi_log(phba, KERN_WARNING,
> - BEISCSI_LOG_INIT | BEISCSI_LOG_EH |
> - BEISCSI_LOG_CONFIG,
> - "BC", "MBX Cmd Failed for Subsys : %d Opcode :
%d
> with Status : %d and Extd_Status : %d\n",
> - mbx_hdr->subsystem,
> - mbx_hdr->opcode,
> - status, addl_status);
> + beiscsi_warn(phba,
> +  BEISCSI_LOG_INIT | BEISCSI_LOG_EH |
> +  BEISCSI_LOG_CONFIG,
> +  "MBX Cmd Failed for Subsys : %d Opcode : %d
with
> Status : %d and Extd_Status : %d\n",
> +  mbx_hdr->subsystem,
> +  mbx_hdr->opcode,
> +  status, addl_status);
>   rc = -EIO;
>   if (status == MCC_STATUS_INSUFFICIENT_BUFFER) {
>   mbx_resp_hdr = (struct be_cmd_resp_hdr *)

Re: [PATCH v6 0/2] Block layer support ZAC/ZBC commands

2016-08-16 Thread Shaun Tancheff 
On Tue, Aug 9, 2016 at 11:38 PM, Damien Le Moal  wrote:
> Shaun,
>
> On 8/10/16 12:58, Shaun Tancheff wrote:
>>
>> On Tue, Aug 9, 2016 at 3:09 AM, Damien Le Moal 
>> wrote:

 On Aug 9, 2016, at 15:47, Hannes Reinecke  wrote:
>>
>>
>> [trim]
>>
> Since disk type == 0 for everything that isn't HM so I would prefer the
> sysfs 'zoned' file just report if the drive is HA or HM.
>
 Okay. So let's put in the 'zoned' attribute the device type:
 'host-managed', 'host-aware', or 'device managed'.
>>>
>>>
>>> I hacked your patches and simply put a "0" or "1" in the sysfs zoned
>>> file.
>>> Any drive that has ZBC/ZAC command support gets a "1", "0" for everything
>>> else. This means that drive managed models are not exposed as zoned block
>>> devices. For HM vs HA differentiation, an application can look at the
>>> device type file since it is already present.
>>>
>>> We could indeed set the "zoned" file to the device type, but HM drives
>>> and
>>> regular drives will both have "0" in it, so no differentiation possible.
>>> The other choice could be the "zoned" bits defined by ZBC, but these
>>> do not define a value for host managed drives, and the drive managed
>>> value
>>> being not "0" could be confusing too. So I settled for a simple 0/1
>>> boolean.
>>
>>
>> This seems good to me.
>
>
> Another option I forgot is for the "zoned" file to indicate the total number
> of zones of the device, and 0 for a non zoned regular block device. That
> would work as well.

Clearly either is sufficient.

> [...]
>>>
>>> Done: I hacked Shaun ioctl code and added finish zone too. The
>>> difference with Shaun initial code is that the ioctl are propagated down
>>> to
>>> the driver (__blkdev_driver_ioctl -> sd_ioctl) so that there is no need
>>> for
>>> BIO request definition for the zone operations. So a lot less code added.
>>
>>
>> The purpose of the BIO flags is not to enable the ioctls so much as
>> the other way round. Creating BIO op's is to enable issuing ZBC
>> commands from device mapper targets and file systems without some
>> heinous ioctl hacks.
>> Making the resulting block layer interfaces available via ioctls is just a
>> reasonable way to exercise the code ... or that was my intent.
>
>
> Yes, I understood your code. However, since (or if) we keep the zone
> information in the RB-tree cache, there is no need for the report zone
> operation BIO interface. Same for reset write pointer by keeping the mapping
> to discard. blk_lookup_zone can be used in kernel as a report zone BIO
> replacement and works as well for the report zone ioctl implementation. For
> reset, there is blkdev_issue_discrad in kernel, and the reset zone ioctl
> becomes equivalent to BLKDISCARD ioctl. These are simple. Open, close and
> finish zone remains. For these, adding the BIO interface seemed an overkill.
> Hence my choice of propagating the ioctl to the driver.
> This is debatable of course, and adding an in-kernel interface is not hard:
> we can implement blk_open_zone, blk_close_zone and blk_finish_zone using
> __blkdev_driver_ioctl. That looks clean to me.

Uh. I would call that "heinous" ioctl hacks myself. Kernel -> User API
-> Kernel
is not really a good designed IMO.

> Overall, my concern with the BIO based interface for the ZBC commands is
> that it adds one flag for each command, which is not really the philosophy
> of the interface and potentially opens the door for more such
> implementations in the future with new standards and new commands coming up.
> Clearly that is not a sustainable path. So I think that a more specific
> interface for these zone operations is a better choice. That is consistent
> with what happens with the tons of ATA and SCSI commands not actually doing
> data I/Os (mode sense, log pages, SMART, etc). All these do not use BIOs and
> are processed as request REQ_TYPE_BLOCK_PC.

Part of the reason for following on Mike Christie's bio op/flags cleanup was to
make these op's. The advantage of being added as ops is that there is only
1 extra bit need (not 4 or 5 bits for flags). The other reason for being
promoted into the block layer as commands is because it seems to me
to make sense that these abstractions could be allowed to be passed through
a DM layer and be handled by a files system.

>>> The ioctls do not mimic exactly the ZBC standard. For instance, there is
>>> no
>>> reporting options for report zones, nor is the "all" bit supported for
>>> open,
>>> close or finish zone commands. But the information provided on zones is
>>> complete
>>> and maps to the standard definitions.
>>
>>
>> For the reporting options I have planned to reuse the stream_id in
>> struct bio when that is formalized. There are certainly other places in
>> struct bio to stuff a few extra bits ...

Sorry I was confused here. I was under the impression you were talking
about one of my patches when you seem to have been talking about