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@xxxxxxxxxxxxxxxxxxx> > 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@xxxxxxxxxxxxxxxxxxx> > Acked-by: Jens Axboe <jens.axboe@xxxxxxxxxx> > Acked-by: Douglas Gilbert <dougg@xxxxxxxxxx> > Signed-off-by: James Bottomley <James.Bottomley@xxxxxxxxxxxx> > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > 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
