* [PATCH] nfsd: make nfsd4_scsi_identify_device retry with a larger buffer @ 2018-05-07 13:01 Scott Mayhew 2018-05-07 18:45 ` J. Bruce Fields 0 siblings, 1 reply; 4+ messages in thread From: Scott Mayhew @ 2018-05-07 13:01 UTC (permalink / raw) To: bfields, jlayton; +Cc: linux-nfs nfsd4_scsi_identify_device() performs a single IDENTIFY command for the device identification VPD page using a small buffer. If the reply is too large to fit in this buffer then the GETDEVICEINFO reply will not contain any info for the SCSI volume aside from the registration key. This can happen for example if the device has descriptors using long SCSI name strings. When the initial reply from the device indicates a larger buffer is needed, retry once using the page length from that reply. Signed-off-by: Scott Mayhew <smayhew@redhat.com> --- fs/nfsd/blocklayout.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/fs/nfsd/blocklayout.c b/fs/nfsd/blocklayout.c index 70b8bf7..0cd9249 100644 --- a/fs/nfsd/blocklayout.c +++ b/fs/nfsd/blocklayout.c @@ -216,13 +216,14 @@ static int nfsd4_scsi_identify_device(struct block_device *bdev, struct request_queue *q = bdev->bd_disk->queue; struct request *rq; struct scsi_request *req; - size_t bufflen = 252, len, id_len; + size_t bufflen = 252, maxlen = 65532, len, id_len; u8 *buf, *d, type, assoc; - int error; + int retries = 1, error; if (WARN_ON_ONCE(!blk_queue_scsi_passthrough(q))) return -EINVAL; +again: buf = kzalloc(bufflen, GFP_KERNEL); if (!buf) return -ENOMEM; @@ -255,9 +256,16 @@ static int nfsd4_scsi_identify_device(struct block_device *bdev, len = (buf[2] << 8) + buf[3] + 4; if (len > bufflen) { - pr_err("pNFS: INQUIRY 0x83 response invalid (len = %zd)\n", - len); - goto out_put_request; + if (len < maxlen && retries--) { + blk_put_request(rq); + kfree(buf); + bufflen = len; + goto again; + } else { + pr_err("pNFS: INQUIRY 0x83 response invalid (len = %zd)\n", + len); + goto out_put_request; + } } d = buf + 4; -- 2.9.5 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] nfsd: make nfsd4_scsi_identify_device retry with a larger buffer 2018-05-07 13:01 [PATCH] nfsd: make nfsd4_scsi_identify_device retry with a larger buffer Scott Mayhew @ 2018-05-07 18:45 ` J. Bruce Fields 2018-05-07 20:17 ` Scott Mayhew 0 siblings, 1 reply; 4+ messages in thread From: J. Bruce Fields @ 2018-05-07 18:45 UTC (permalink / raw) To: Scott Mayhew; +Cc: jlayton, linux-nfs On Mon, May 07, 2018 at 09:01:31AM -0400, Scott Mayhew wrote: > nfsd4_scsi_identify_device() performs a single IDENTIFY command for the > device identification VPD page using a small buffer. If the reply is > too large to fit in this buffer then the GETDEVICEINFO reply will not > contain any info for the SCSI volume aside from the registration key. > This can happen for example if the device has descriptors using long > SCSI name strings. > > When the initial reply from the device indicates a larger buffer is > needed, retry once using the page length from that reply. Looks good, but how did you choose 65532 as the maximum? Does the scsi layer give us any maximum? Also, I don't know why this is bothering me, but: I'd prefer "buflen" to "bufflen". --b. > > Signed-off-by: Scott Mayhew <smayhew@redhat.com> > --- > fs/nfsd/blocklayout.c | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/fs/nfsd/blocklayout.c b/fs/nfsd/blocklayout.c > index 70b8bf7..0cd9249 100644 > --- a/fs/nfsd/blocklayout.c > +++ b/fs/nfsd/blocklayout.c > @@ -216,13 +216,14 @@ static int nfsd4_scsi_identify_device(struct block_device *bdev, > struct request_queue *q = bdev->bd_disk->queue; > struct request *rq; > struct scsi_request *req; > - size_t bufflen = 252, len, id_len; > + size_t bufflen = 252, maxlen = 65532, len, id_len; > u8 *buf, *d, type, assoc; > - int error; > + int retries = 1, error; > > if (WARN_ON_ONCE(!blk_queue_scsi_passthrough(q))) > return -EINVAL; > > +again: > buf = kzalloc(bufflen, GFP_KERNEL); > if (!buf) > return -ENOMEM; > @@ -255,9 +256,16 @@ static int nfsd4_scsi_identify_device(struct block_device *bdev, > > len = (buf[2] << 8) + buf[3] + 4; > if (len > bufflen) { > - pr_err("pNFS: INQUIRY 0x83 response invalid (len = %zd)\n", > - len); > - goto out_put_request; > + if (len < maxlen && retries--) { > + blk_put_request(rq); > + kfree(buf); > + bufflen = len; > + goto again; > + } else { > + pr_err("pNFS: INQUIRY 0x83 response invalid (len = %zd)\n", > + len); > + goto out_put_request; > + } > } > > d = buf + 4; > -- > 2.9.5 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] nfsd: make nfsd4_scsi_identify_device retry with a larger buffer 2018-05-07 18:45 ` J. Bruce Fields @ 2018-05-07 20:17 ` Scott Mayhew 2018-05-10 14:16 ` J. Bruce Fields 0 siblings, 1 reply; 4+ messages in thread From: Scott Mayhew @ 2018-05-07 20:17 UTC (permalink / raw) To: J. Bruce Fields; +Cc: jlayton, linux-nfs On Mon, 07 May 2018, J. Bruce Fields wrote: > On Mon, May 07, 2018 at 09:01:31AM -0400, Scott Mayhew wrote: > > nfsd4_scsi_identify_device() performs a single IDENTIFY command for the > > device identification VPD page using a small buffer. If the reply is > > too large to fit in this buffer then the GETDEVICEINFO reply will not > > contain any info for the SCSI volume aside from the registration key. > > This can happen for example if the device has descriptors using long > > SCSI name strings. > > > > When the initial reply from the device indicates a larger buffer is > > needed, retry once using the page length from that reply. > > Looks good, but how did you choose 65532 as the maximum? Does the scsi > layer give us any maximum? So I wasn't quite sure about that. The allocation length field for an INQUIRY command is two bytes. There are various fields that the SPC-3 doc says have to be a multiple of 4, but I couldn't find anything that says whether that applies to the allocation length. Since the initial value 252 is a muliple of 4, I figured it wouldn't hurt anything to have the maximum be a multiple of 4 too. OTOH if you look at scsi_get_vpd_buf() in drivers/scsi/scsi.c, the initial buffer size is SCSI_VPD_PG_LEN, which is 255. -Scott > > Also, I don't know why this is bothering me, but: I'd prefer "buflen" to > "bufflen". > > --b. > > > > > Signed-off-by: Scott Mayhew <smayhew@redhat.com> > > --- > > fs/nfsd/blocklayout.c | 18 +++++++++++++----- > > 1 file changed, 13 insertions(+), 5 deletions(-) > > > > diff --git a/fs/nfsd/blocklayout.c b/fs/nfsd/blocklayout.c > > index 70b8bf7..0cd9249 100644 > > --- a/fs/nfsd/blocklayout.c > > +++ b/fs/nfsd/blocklayout.c > > @@ -216,13 +216,14 @@ static int nfsd4_scsi_identify_device(struct block_device *bdev, > > struct request_queue *q = bdev->bd_disk->queue; > > struct request *rq; > > struct scsi_request *req; > > - size_t bufflen = 252, len, id_len; > > + size_t bufflen = 252, maxlen = 65532, len, id_len; > > u8 *buf, *d, type, assoc; > > - int error; > > + int retries = 1, error; > > > > if (WARN_ON_ONCE(!blk_queue_scsi_passthrough(q))) > > return -EINVAL; > > > > +again: > > buf = kzalloc(bufflen, GFP_KERNEL); > > if (!buf) > > return -ENOMEM; > > @@ -255,9 +256,16 @@ static int nfsd4_scsi_identify_device(struct block_device *bdev, > > > > len = (buf[2] << 8) + buf[3] + 4; > > if (len > bufflen) { > > - pr_err("pNFS: INQUIRY 0x83 response invalid (len = %zd)\n", > > - len); > > - goto out_put_request; > > + if (len < maxlen && retries--) { > > + blk_put_request(rq); > > + kfree(buf); > > + bufflen = len; > > + goto again; > > + } else { > > + pr_err("pNFS: INQUIRY 0x83 response invalid (len = %zd)\n", > > + len); > > + goto out_put_request; > > + } > > } > > > > d = buf + 4; > > -- > > 2.9.5 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] nfsd: make nfsd4_scsi_identify_device retry with a larger buffer 2018-05-07 20:17 ` Scott Mayhew @ 2018-05-10 14:16 ` J. Bruce Fields 0 siblings, 0 replies; 4+ messages in thread From: J. Bruce Fields @ 2018-05-10 14:16 UTC (permalink / raw) To: Scott Mayhew; +Cc: jlayton, linux-nfs On Mon, May 07, 2018 at 04:17:40PM -0400, Scott Mayhew wrote: > On Mon, 07 May 2018, J. Bruce Fields wrote: > > > On Mon, May 07, 2018 at 09:01:31AM -0400, Scott Mayhew wrote: > > > nfsd4_scsi_identify_device() performs a single IDENTIFY command for the > > > device identification VPD page using a small buffer. If the reply is > > > too large to fit in this buffer then the GETDEVICEINFO reply will not > > > contain any info for the SCSI volume aside from the registration key. > > > This can happen for example if the device has descriptors using long > > > SCSI name strings. > > > > > > When the initial reply from the device indicates a larger buffer is > > > needed, retry once using the page length from that reply. > > > > Looks good, but how did you choose 65532 as the maximum? Does the scsi > > layer give us any maximum? > > So I wasn't quite sure about that. The allocation length field for an > INQUIRY command is two bytes. There are various fields that the SPC-3 > doc says have to be a multiple of 4, but I couldn't find anything that > says whether that applies to the allocation length. Since the initial > value 252 is a muliple of 4, I figured it wouldn't hurt anything to have > the maximum be a multiple of 4 too. > > OTOH if you look at scsi_get_vpd_buf() in drivers/scsi/scsi.c, the > initial buffer size is SCSI_VPD_PG_LEN, which is 255. OK. Well, till we have some input from a scsi expert, would you mind just adding a brief summary of the above as a code comment? --b. > > -Scott > > > > > Also, I don't know why this is bothering me, but: I'd prefer "buflen" to > > "bufflen". > > > > --b. > > > > > > > > Signed-off-by: Scott Mayhew <smayhew@redhat.com> > > > --- > > > fs/nfsd/blocklayout.c | 18 +++++++++++++----- > > > 1 file changed, 13 insertions(+), 5 deletions(-) > > > > > > diff --git a/fs/nfsd/blocklayout.c b/fs/nfsd/blocklayout.c > > > index 70b8bf7..0cd9249 100644 > > > --- a/fs/nfsd/blocklayout.c > > > +++ b/fs/nfsd/blocklayout.c > > > @@ -216,13 +216,14 @@ static int nfsd4_scsi_identify_device(struct block_device *bdev, > > > struct request_queue *q = bdev->bd_disk->queue; > > > struct request *rq; > > > struct scsi_request *req; > > > - size_t bufflen = 252, len, id_len; > > > + size_t bufflen = 252, maxlen = 65532, len, id_len; > > > u8 *buf, *d, type, assoc; > > > - int error; > > > + int retries = 1, error; > > > > > > if (WARN_ON_ONCE(!blk_queue_scsi_passthrough(q))) > > > return -EINVAL; > > > > > > +again: > > > buf = kzalloc(bufflen, GFP_KERNEL); > > > if (!buf) > > > return -ENOMEM; > > > @@ -255,9 +256,16 @@ static int nfsd4_scsi_identify_device(struct block_device *bdev, > > > > > > len = (buf[2] << 8) + buf[3] + 4; > > > if (len > bufflen) { > > > - pr_err("pNFS: INQUIRY 0x83 response invalid (len = %zd)\n", > > > - len); > > > - goto out_put_request; > > > + if (len < maxlen && retries--) { > > > + blk_put_request(rq); > > > + kfree(buf); > > > + bufflen = len; > > > + goto again; > > > + } else { > > > + pr_err("pNFS: INQUIRY 0x83 response invalid (len = %zd)\n", > > > + len); > > > + goto out_put_request; > > > + } > > > } > > > > > > d = buf + 4; > > > -- > > > 2.9.5 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-05-10 14:16 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-05-07 13:01 [PATCH] nfsd: make nfsd4_scsi_identify_device retry with a larger buffer Scott Mayhew 2018-05-07 18:45 ` J. Bruce Fields 2018-05-07 20:17 ` Scott Mayhew 2018-05-10 14:16 ` J. Bruce Fields
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).