Re: [PATCH RESEND 2/4] scsi: sg: implement BLKSSZGET

[Alan Stern <stern@rowland.harvard.edu>]

On Fri, Sep 11, 2020 at 01:52:07PM -0400, Douglas Gilbert wrote:
> On 2020-09-11 2:48 a.m., Christoph Hellwig wrote:
> > On Fri, Sep 11, 2020 at 10:52:19AM +0800, Tom Yan wrote:
> > > > How is that an advantage?  Applications that works with block devices
> > > > don't really work with a magic passthrough character device.
> > > 
> > > You must assume that there are applications already assuming that
> > > work. (And it will, at least in some cases, if this series get
> > > merged.)
> > 
> > Why "must" I assume that?
> > 
> > > And you have not been giving me a solid point anyway, as I said, it's
> > > just queue_*() at the end of the day; regardless of whether those
> > > would work in all sg cases, we have been using them in the sg driver
> > > anyway.
> > > 
> > > And it's not like we have to guarantee that (the) ioctls can work in
> > > every case anyway, right? (Especially when they aren't named SG_*).
> > 
> > No.  While it is unfortunte we have all kinds of cases of ioctls working
> > differnetly on different devices.
> > 
> > > 
> > > I mean, what's even your point? How do you propose we fix this?
> > 
> > I propose to not "fix" anything, because nothing is broken except for
> > maybe a lack of documentation.
> 
> Alan Stern are you reading this thread? Why do I ask, you may ask?
> Because 'git blame' fingers you:
> 
> vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
> 
> commit 44ec95425c1d9dce6e4638c29e4362cfb44814e7
> Author: Alan Stern <stern@rowland.harvard.edu>
> Date:   Tue Feb 20 11:01:57 2007 -0500
> 
>     [SCSI] sg: cap reserved_size values at max_sectors
> 
>     This patch (as857) modifies the SG_GET_RESERVED_SIZE and
>     SG_SET_RESERVED_SIZE ioctls in the sg driver, capping the values at
>     the device's request_queue's max_sectors value.  This will permit
>     cdrecord to obtain a legal value for the maximum transfer length,
>     fixing Bugzilla #7026.
> 
>     The patch also caps the initial reserved_size value.  There's no
>     reason to have a reserved buffer larger than max_sectors, since it
>     would be impossible to use the extra space.
> 
>     The corresponding ioctls in the block layer are modified similarly,
>     and the initial value for the reserved_size is set as large as
>     possible.  This will effectively make it default to max_sectors.
>     Note that the actual value is meaningless anyway, since block devices
>     don't have a reserved buffer.
> 
>     Finally, the BLKSECTGET ioctl is added to sg, so that there will be a
>     uniform way for users to determine the actual max_sectors value for
>     any raw SCSI transport.
> 
>     Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
>     Acked-by: Jens Axboe <jens.axboe@oracle.com>
>     Acked-by: Douglas Gilbert <dougg@torque.net>
>     Signed-off-by: James Bottomley <James.Bottomley@SteelEye.com>
> 
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> Oops, I ack-ed this patch from 2007:-)

The Bugzilla entry it talks about is from 2006!

>  Anyway it would seem BLKSECTGET ioctl
> was meant to be a "uniform way to determine the actual max_sectors value for
> any raw SCSI transport."

Right.  The question at hand was: Given an open file descriptor for an 
SG device, how can a program determine the largest amount it can send in 
a single transfer?  This ioctl seemed to be the best answer.

See comment #26 (https://bugzilla.kernel.org/show_bug.cgi?id=7026#c26) 
and following for the viewpoint of the notoriously prickly author of 
cdrecord.

>  Given that the initial implementation of BLKSECTGET
> now seems to be at odds with other implementations, what should we do?
> 
> It is possible that it was correct on 2007 and the BLKSECTGET ioctl has
> changed elsewhere but failed to fix the sg driver's implementation.

Could be.  Also, I'm not sure how careful people were back then to 
distinguish between logical and physical sector sizes.

> If I get a vote then it would be for Tom Yan's approach: reduce entropy or
> it will overwhelm us :-)
> 
> 
> So Christoph, it IS documented, both in the above commit message and:
>    https://doug-gilbert.github.io/sg_v40.html
> 
> in Table 8. So please stop with your "maybe a lack of documentation" line.

My vote is not to change an interface which a program like cdrecord may 
currently rely on.

I can understand Christoph's point about documentation.  It would be 
good to have something in the actual kernel source, rather than in the 
history or somebody's github files.

Alan Stern