Re: [PATCH] Block layer: separate out queue-oriented ioctls
On Thu, 2007-04-26 at 11:04 -0400, Alan Stern wrote: > On Thu, 26 Apr 2007, Joerg Schilling wrote: > > > Any chance to that this bug will be fixed anytime soon? > > > > The Bug has been reported February 2004 but is still not fixed in sg.c > > Is Linux development really so slow? > > Douglas Gilbert has signed off on the patch. There doesn't appear to have > been any change to the scsi-misc-2.6 GIT tree recently, but hopefully the > changes will appear in the 2.6.22 kernel. > > James, is that correct? Yes, it's in my local tree James - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Block layer: separate out queue-oriented ioctls
On Thu, 26 Apr 2007, Joerg Schilling wrote: > Any chance to that this bug will be fixed anytime soon? > > The Bug has been reported February 2004 but is still not fixed in sg.c > Is Linux development really so slow? Douglas Gilbert has signed off on the patch. There doesn't appear to have been any change to the scsi-misc-2.6 GIT tree recently, but hopefully the changes will appear in the 2.6.22 kernel. James, is that correct? Alan Stern - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Block layer: separate out queue-oriented ioctls
Any chance to that this bug will be fixed anytime soon?
The Bug has been reported February 2004 but is still not fixed in sg.c
Is Linux development really so slow?
Douglas Gilbert <[EMAIL PROTECTED]> wrote:
> Alan,
> The SG_GET_RESERVED_SIZE ioctl is also defined in
> the block layer, see block/scsi_ioctl.c .
> I suspect it is just a kludge to fool cdrecord that it
> is talking to a sg device. [One of many kludges in the
> block SG_IO ioctl implementation to that end.]
> So perhaps the block layer versions of SG_SET_RESERVED_SIZE
> and SG_GET_RESERVED_SIZE need to be similarly capped.
> Actually I think that I would default SG_GET_RESERVED_SIZE to
> request_queue->max_sectors * 512 in the block layer
> implementation (as there is no "reserve buffer" associated
> with a block device).
>
>
>
> The idea of a reserved buffer may live on in bsg as experience
> with sg has shown that it is the fastest way to do (mmap-ed) IO.
> Having one reserved buffer per file descriptor means not
> having to create and tear down a scatter gather list
> per IO. [Having a pool of such lists would be even better.]
> Until optical storage needs 10 times its current datarates
> then cdrecord will not need this mechanism.
>
>
> Doug Gilbert
>
>
> > Index: usb-2.6/drivers/scsi/sg.c
> > ===
> > --- usb-2.6.orig/drivers/scsi/sg.c
> > +++ usb-2.6/drivers/scsi/sg.c
> > @@ -917,6 +917,8 @@ sg_ioctl(struct inode *inode, struct fil
> > return result;
> > if (val < 0)
> > return -EINVAL;
> > + if (val > sdp->device->request_queue->max_sectors * 512)
> > + return -EOVERFLOW;
> > if (val != sfp->reserve.bufflen) {
> > if (sg_res_in_use(sfp) || sfp->mmap_called)
> > return -EBUSY;
> > @@ -925,7 +927,8 @@ sg_ioctl(struct inode *inode, struct fil
> > }
> > return 0;
> > case SG_GET_RESERVED_SIZE:
> > - val = (int) sfp->reserve.bufflen;
> > + val = min_t(int, sfp->reserve.bufflen,
> > + sdp->device->request_queue->max_sectors * 512);
> > return put_user(val, ip);
> > case SG_SET_COMMAND_Q:
> > result = get_user(val, ip);
> >
Jörg
--
EMail:[EMAIL PROTECTED] (home) Jörg Schilling D-13353 Berlin
[EMAIL PROTECTED](uni)
[EMAIL PROTECTED] (work) Blog: http://schily.blogspot.com/
URL: http://cdrecord.berlios.de/old/private/ ftp://ftp.berlios.de/pub/schily
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Block layer: separate out queue-oriented ioctls Re: [PATCH] Block layer: separate out queue-oriented ioctls
On Mon, 2007-04-02 at 16:23 +0200, Jens Axboe wrote: > FWIW, it had my ack, I think we were just waiting for Doug to ack the sg > bits. And there's really nothing I can do (well, except write the thing) since the changes are not in any SCSI pieces I maintain directly ... they're block and sg. James - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Block layer: separate out queue-oriented ioctls Re: [PATCH] Block layer: separate out queue-oriented ioctls
On Mon, Apr 02 2007, Alan Stern wrote: > On Mon, 2 Apr 2007, Joerg Schilling wrote: > > > Hi, > > > > this is a repost as I like to know the current state of the problem... > > > > The USB DMA size problem is known to exist on Linux since February 2004. > > I am still in hope that there will be a fix soon. > > Me too. I submitted the most recent version of the patch (labelled as857) > over a month ago and have received essentially no feedback on it. > > Douglas, James... What's the story? FWIW, it had my ack, I think we were just waiting for Doug to ack the sg bits. -- Jens Axboe - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Block layer: separate out queue-oriented ioctls Re: [PATCH] Block layer: separate out queue-oriented ioctls
On Mon, 2 Apr 2007, Joerg Schilling wrote: > Hi, > > this is a repost as I like to know the current state of the problem... > > The USB DMA size problem is known to exist on Linux since February 2004. > I am still in hope that there will be a fix soon. Me too. I submitted the most recent version of the patch (labelled as857) over a month ago and have received essentially no feedback on it. Douglas, James... What's the story? Alan Stern - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Block layer: separate out queue-oriented ioctls Re: [PATCH] Block layer: separate out queue-oriented ioctls
Hi, this is a repost as I like to know the current state of the problem... The USB DMA size problem is known to exist on Linux since February 2004. I am still in hope that there will be a fix soon. /*--*/ Alan Stern <[EMAIL PROTECTED]> wrote: > Well, if Doug wants to reduce the value returned by SG_GET_RESERVED_SIZE, > it's okay with me. An advantage of doing this is that older versions of > cdrecord would then work correctly. > > However you don't seem to realize that people can use programs like > cdrecord with devices whose drivers don't support SG_GET_RESERVED_SIZE -- > because that ioctl works only with sg. Programs would have to try > SG_GET_RESERVED_SIZE and if it faied, then try BLKSECTGET. Is there any reason not to have one single ioctl for one basic feature? > Remember also, the "reserved size" is _not_ the maximum allowed size of a > DMA transfer. Rather, it is the size of an internal buffer maintained by > sg. It's legal to do an I/O transfer larger than the "reserved size", but > it is not legal to do an I/O transfer larger than max_sectors. At the time the call SG_GET_RESERVED_SIZE has been discussed/defined, we did originally agree that the max value should be limited to what the HW allows as DMA size. This is why I did originally files a bug against SG_GET_RESERVED_SIZE. /*--*/ Jörg -- EMail:[EMAIL PROTECTED] (home) Jörg Schilling D-13353 Berlin [EMAIL PROTECTED](uni) [EMAIL PROTECTED] (work) Blog: http://schily.blogspot.com/ URL: http://cdrecord.berlios.de/old/private/ ftp://ftp.berlios.de/pub/schily - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Block layer: separate out queue-oriented ioctls
On Mon, 19 Feb 2007, Douglas Gilbert wrote: > > Come to think of it, the reserved_size value used when a new sg device is > > created should also be capped at max_sectors * 512. Agreed? I can't see > > any reason for ever having a larger buffer -- it would be impossible to > > make use of the extra space. > > Alan, > That depends whether or not max_sectors can be changed > (via sysfs) subsequent to a sg device being created. > And I think it can. > > # ls -l /sys/block/sdc/queue/ > total 0 > drwxr-xr-x 2 root root0 Feb 19 18:29 iosched > -r--r--r-- 1 root root 4096 Feb 19 23:41 max_hw_sectors_kb > -rw-r--r-- 1 root root 4096 Feb 19 23:41 max_sectors_kb > -rw-r--r-- 1 root root 4096 Feb 19 23:41 nr_requests > -rw-r--r-- 1 root root 4096 Feb 19 23:41 read_ahead_kb > -rw-r--r-- 1 root root 4096 Feb 19 23:41 scheduler Yes, it definitely can be changed. > # cat max_hw_sectors_kb > max_sectors_kb > > ... is the real maximum if the LLD that set max_hw_sectors_kb > is to be believed (actually it is often a finger in > the wind). That's why my patch computes the minimum value every time the GET_RESERVED_SIZE ioctl runs -- in case max_sectors has changed. If the user decides to increase max_sectors, then the reserved_size can be increased immediately afterward. This shouldn't cause any problems. I will submit a revised patch shortly, on the linux-scsi list. Alan Stern - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Block layer: separate out queue-oriented ioctls
Alan Stern wrote: > On Mon, 19 Feb 2007, Douglas Gilbert wrote: > >> Alan, >> The SG_GET_RESERVED_SIZE ioctl is also defined in >> the block layer, see block/scsi_ioctl.c . > > Ah, I didn't know that. (Or more likely, I used to know and have since > forgotten.) Thanks for pointing it out. > >> I suspect it is just a kludge to fool cdrecord that it >> is talking to a sg device. [One of many kludges in the >> block SG_IO ioctl implementation to that end.] >> So perhaps the block layer versions of SG_SET_RESERVED_SIZE >> and SG_GET_RESERVED_SIZE need to be similarly capped. > > Yes. In fact one of them already is, but the other should be too. > >> Actually I think that I would default SG_GET_RESERVED_SIZE to >> request_queue->max_sectors * 512 in the block layer >> implementation (as there is no "reserve buffer" associated >> with a block device). > > Okay. > > Come to think of it, the reserved_size value used when a new sg device is > created should also be capped at max_sectors * 512. Agreed? I can't see > any reason for ever having a larger buffer -- it would be impossible to > make use of the extra space. Alan, That depends whether or not max_sectors can be changed (via sysfs) subsequent to a sg device being created. And I think it can. # ls -l /sys/block/sdc/queue/ total 0 drwxr-xr-x 2 root root0 Feb 19 18:29 iosched -r--r--r-- 1 root root 4096 Feb 19 23:41 max_hw_sectors_kb -rw-r--r-- 1 root root 4096 Feb 19 23:41 max_sectors_kb -rw-r--r-- 1 root root 4096 Feb 19 23:41 nr_requests -rw-r--r-- 1 root root 4096 Feb 19 23:41 read_ahead_kb -rw-r--r-- 1 root root 4096 Feb 19 23:41 scheduler # cat max_hw_sectors_kb > max_sectors_kb ... is the real maximum if the LLD that set max_hw_sectors_kb is to be believed (actually it is often a finger in the wind). Doug Gilbert - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Block layer: separate out queue-oriented ioctls
On Mon, 19 Feb 2007, Douglas Gilbert wrote: > Alan, > The SG_GET_RESERVED_SIZE ioctl is also defined in > the block layer, see block/scsi_ioctl.c . Ah, I didn't know that. (Or more likely, I used to know and have since forgotten.) Thanks for pointing it out. > I suspect it is just a kludge to fool cdrecord that it > is talking to a sg device. [One of many kludges in the > block SG_IO ioctl implementation to that end.] > So perhaps the block layer versions of SG_SET_RESERVED_SIZE > and SG_GET_RESERVED_SIZE need to be similarly capped. Yes. In fact one of them already is, but the other should be too. > Actually I think that I would default SG_GET_RESERVED_SIZE to > request_queue->max_sectors * 512 in the block layer > implementation (as there is no "reserve buffer" associated > with a block device). Okay. Come to think of it, the reserved_size value used when a new sg device is created should also be capped at max_sectors * 512. Agreed? I can't see any reason for ever having a larger buffer -- it would be impossible to make use of the extra space. Alan Stern - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Block layer: separate out queue-oriented ioctls
Alan Stern wrote:
> On Mon, 19 Feb 2007, Joerg Schilling wrote:
>
>> Alan Stern <[EMAIL PROTECTED]> wrote:
>>
>>> Well, if Doug wants to reduce the value returned by SG_GET_RESERVED_SIZE,
>>> it's okay with me. An advantage of doing this is that older versions of
>>> cdrecord would then work correctly.
>>>
>>> However you don't seem to realize that people can use programs like
>>> cdrecord with devices whose drivers don't support SG_GET_RESERVED_SIZE --
>>> because that ioctl works only with sg. Programs would have to try
>>> SG_GET_RESERVED_SIZE and if it faied, then try BLKSECTGET.
>> Is there any reason not to have one single ioctl for one basic feature?
>
> Indeed there is not. That's what I wrote in an earlier email:
>
> "There should be one single ioctl which can be applied uniformly to all
> CD-type devices (in fact, to all devices using a request_queue) to learn
> max_sectors. This rules out using SG_GET_RESERVED_SIZE."
>
>>> Remember also, the "reserved size" is _not_ the maximum allowed size of a
>>> DMA transfer. Rather, it is the size of an internal buffer maintained by
>>> sg. It's legal to do an I/O transfer larger than the "reserved size", but
>>> it is not legal to do an I/O transfer larger than max_sectors.
>> At the time the call SG_GET_RESERVED_SIZE has been discussed/defined, we did
>> originally agree that the max value should be limited to what the HW allows
>> as DMA size. This is why I did originally files a bug against
>> SG_GET_RESERVED_SIZE.
>
> How do you feel about the patch below, either in addition to or instead of
> the previous patch?
Alan,
The SG_GET_RESERVED_SIZE ioctl is also defined in
the block layer, see block/scsi_ioctl.c .
I suspect it is just a kludge to fool cdrecord that it
is talking to a sg device. [One of many kludges in the
block SG_IO ioctl implementation to that end.]
So perhaps the block layer versions of SG_SET_RESERVED_SIZE
and SG_GET_RESERVED_SIZE need to be similarly capped.
Actually I think that I would default SG_GET_RESERVED_SIZE to
request_queue->max_sectors * 512 in the block layer
implementation (as there is no "reserve buffer" associated
with a block device).
The idea of a reserved buffer may live on in bsg as experience
with sg has shown that it is the fastest way to do (mmap-ed) IO.
Having one reserved buffer per file descriptor means not
having to create and tear down a scatter gather list
per IO. [Having a pool of such lists would be even better.]
Until optical storage needs 10 times its current datarates
then cdrecord will not need this mechanism.
Doug Gilbert
> Index: usb-2.6/drivers/scsi/sg.c
> ===
> --- usb-2.6.orig/drivers/scsi/sg.c
> +++ usb-2.6/drivers/scsi/sg.c
> @@ -917,6 +917,8 @@ sg_ioctl(struct inode *inode, struct fil
> return result;
> if (val < 0)
> return -EINVAL;
> + if (val > sdp->device->request_queue->max_sectors * 512)
> + return -EOVERFLOW;
> if (val != sfp->reserve.bufflen) {
> if (sg_res_in_use(sfp) || sfp->mmap_called)
> return -EBUSY;
> @@ -925,7 +927,8 @@ sg_ioctl(struct inode *inode, struct fil
> }
> return 0;
> case SG_GET_RESERVED_SIZE:
> - val = (int) sfp->reserve.bufflen;
> + val = min_t(int, sfp->reserve.bufflen,
> + sdp->device->request_queue->max_sectors * 512);
> return put_user(val, ip);
> case SG_SET_COMMAND_Q:
> result = get_user(val, ip);
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Block layer: separate out queue-oriented ioctls
On Mon, 19 Feb 2007, Joerg Schilling wrote:
> Alan Stern <[EMAIL PROTECTED]> wrote:
>
> > Well, if Doug wants to reduce the value returned by SG_GET_RESERVED_SIZE,
> > it's okay with me. An advantage of doing this is that older versions of
> > cdrecord would then work correctly.
> >
> > However you don't seem to realize that people can use programs like
> > cdrecord with devices whose drivers don't support SG_GET_RESERVED_SIZE --
> > because that ioctl works only with sg. Programs would have to try
> > SG_GET_RESERVED_SIZE and if it faied, then try BLKSECTGET.
>
> Is there any reason not to have one single ioctl for one basic feature?
Indeed there is not. That's what I wrote in an earlier email:
"There should be one single ioctl which can be applied uniformly to all
CD-type devices (in fact, to all devices using a request_queue) to learn
max_sectors. This rules out using SG_GET_RESERVED_SIZE."
> > Remember also, the "reserved size" is _not_ the maximum allowed size of a
> > DMA transfer. Rather, it is the size of an internal buffer maintained by
> > sg. It's legal to do an I/O transfer larger than the "reserved size", but
> > it is not legal to do an I/O transfer larger than max_sectors.
>
> At the time the call SG_GET_RESERVED_SIZE has been discussed/defined, we did
> originally agree that the max value should be limited to what the HW allows
> as DMA size. This is why I did originally files a bug against
> SG_GET_RESERVED_SIZE.
How do you feel about the patch below, either in addition to or instead of
the previous patch?
Alan Stern
Index: usb-2.6/drivers/scsi/sg.c
===
--- usb-2.6.orig/drivers/scsi/sg.c
+++ usb-2.6/drivers/scsi/sg.c
@@ -917,6 +917,8 @@ sg_ioctl(struct inode *inode, struct fil
return result;
if (val < 0)
return -EINVAL;
+ if (val > sdp->device->request_queue->max_sectors * 512)
+ return -EOVERFLOW;
if (val != sfp->reserve.bufflen) {
if (sg_res_in_use(sfp) || sfp->mmap_called)
return -EBUSY;
@@ -925,7 +927,8 @@ sg_ioctl(struct inode *inode, struct fil
}
return 0;
case SG_GET_RESERVED_SIZE:
- val = (int) sfp->reserve.bufflen;
+ val = min_t(int, sfp->reserve.bufflen,
+ sdp->device->request_queue->max_sectors * 512);
return put_user(val, ip);
case SG_SET_COMMAND_Q:
result = get_user(val, ip);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Block layer: separate out queue-oriented ioctls
Alan Stern <[EMAIL PROTECTED]> wrote: > Well, if Doug wants to reduce the value returned by SG_GET_RESERVED_SIZE, > it's okay with me. An advantage of doing this is that older versions of > cdrecord would then work correctly. > > However you don't seem to realize that people can use programs like > cdrecord with devices whose drivers don't support SG_GET_RESERVED_SIZE -- > because that ioctl works only with sg. Programs would have to try > SG_GET_RESERVED_SIZE and if it faied, then try BLKSECTGET. Is there any reason not to have one single ioctl for one basic feature? > Remember also, the "reserved size" is _not_ the maximum allowed size of a > DMA transfer. Rather, it is the size of an internal buffer maintained by > sg. It's legal to do an I/O transfer larger than the "reserved size", but > it is not legal to do an I/O transfer larger than max_sectors. At the time the call SG_GET_RESERVED_SIZE has been discussed/defined, we did originally agree that the max value should be limited to what the HW allows as DMA size. This is why I did originally files a bug against SG_GET_RESERVED_SIZE. Jörg -- EMail:[EMAIL PROTECTED] (home) Jörg Schilling D-13353 Berlin [EMAIL PROTECTED](uni) [EMAIL PROTECTED] (work) Blog: http://schily.blogspot.com/ URL: http://cdrecord.berlios.de/old/private/ ftp://ftp.berlios.de/pub/schily - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Block layer: separate out queue-oriented ioctls
On Sun, 18 Feb 2007, Joerg Schilling wrote: > Alan Stern <[EMAIL PROTECTED]> wrote: > > > > Alternatively the SG_GET_RESERVED_SIZE ioctl could be > > > modified to yield no more than max_sectors*512 . > > > > There should be one single ioctl which can be applied uniformly to all > > CD-type devices (in fact, to all devices using a request_queue) to learn > > max_sectors. This rules out using SG_GET_RESERVED_SIZE. > > This has nothing to do with CD-type devices! > It is related to SCSI tansport. Actually, it isn't even related to SCSI transport; it is related to the request_queue interface. Even for devices which don't use a SCSI transport, the request_queue code limits transfer lengths to max_sectors. > > Furthermore, if you changed SG_GET_RESERVED_SIZE in this way you would > > only increase the confusion. The reserved size isn't directly related to > > the maximum allowed DMA length, and there's no point pretending it is. > > What if it turns out that the reserved size is smaller than max_sectors? > > Then you'd force user programs to do I/O in chunks that were smaller than > > necessary. I take back that last sentence. If the reserved size was smaller than max_sectors then nothing would be changed. > It would not increase confusion but reduce confusion because all > programs would later behave correctly without the need to change them. Well, if Doug wants to reduce the value returned by SG_GET_RESERVED_SIZE, it's okay with me. An advantage of doing this is that older versions of cdrecord would then work correctly. However you don't seem to realize that people can use programs like cdrecord with devices whose drivers don't support SG_GET_RESERVED_SIZE -- because that ioctl works only with sg. Programs would have to try SG_GET_RESERVED_SIZE and if it faied, then try BLKSECTGET. Remember also, the "reserved size" is _not_ the maximum allowed size of a DMA transfer. Rather, it is the size of an internal buffer maintained by sg. It's legal to do an I/O transfer larger than the "reserved size", but it is not legal to do an I/O transfer larger than max_sectors. Alan Stern - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Block layer: separate out queue-oriented ioctls
Alan Stern <[EMAIL PROTECTED]> wrote: > > Alternatively the SG_GET_RESERVED_SIZE ioctl could be > > modified to yield no more than max_sectors*512 . > > There should be one single ioctl which can be applied uniformly to all > CD-type devices (in fact, to all devices using a request_queue) to learn > max_sectors. This rules out using SG_GET_RESERVED_SIZE. This has nothing to do with CD-type devices! It is related to SCSI tansport. > Furthermore, if you changed SG_GET_RESERVED_SIZE in this way you would > only increase the confusion. The reserved size isn't directly related to > the maximum allowed DMA length, and there's no point pretending it is. > What if it turns out that the reserved size is smaller than max_sectors? > Then you'd force user programs to do I/O in chunks that were smaller than > necessary. It would not increase confusion but reduce confusion because all programs would later behave correctly without the need to change them. Jörg -- EMail:[EMAIL PROTECTED] (home) Jörg Schilling D-13353 Berlin [EMAIL PROTECTED](uni) [EMAIL PROTECTED] (work) Blog: http://schily.blogspot.com/ URL: http://cdrecord.berlios.de/old/private/ ftp://ftp.berlios.de/pub/schily - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Block layer: separate out queue-oriented ioctls
On Sat, 17 Feb 2007, Douglas Gilbert wrote: > Jens Axboe wrote: > > On Fri, Feb 16 2007, Alan Stern wrote: > >> From: James Bottomley <[EMAIL PROTECTED]> > >> > >> This patch (as854) separates out the two queue-oriented ioctls from > >> the rest of the block-layer ioctls. The idea is that they should > >> apply to any driver using a request_queue, even if the driver doesn't > >> implement a block-device interface. The prototypical example is the > >> sg driver, to which the patch adds the new interface. > >> > >> This will make it possible for cdrecord and related programs to > >> retrieve reliably the max_sectors value, regardless of whether the > >> user points it to an sr or an sg device. In particular, this will > >> resolve Bugzilla entry #7026. > > > > The block bits are fine with me, the sg calling point is a bit of a sore > > thumb (a char driver calling into block layer ioctls) though. So the > > block layer bits are certainly ok with me, if Doug acks the sg bit I'll > > merge everything up. > > > > (patch left below) > > Does this need to be in the sg driver? Something like it does. Otherwise cdrecord (or any other sg user) has no way to retrieve the max_sectors value for sg devices. > What is the hardware sector size of a SES or OSD device? I have no idea, and I don't see why it is relevant. (I don't even know what "SES" and "OSD" refer to.) > As for the max_sector variable, wouldn't it be better > to generate a new ioctl that yielded the limit in bytes? Maybe. I wouldn't mind doing it that way. But we would have to leave in the old ioctl, and we probably would still want it to be usable for sg devices. Not to mention that it would be silly to have two ioctls which always return exactly the same values except for a factor of 512. > Making a driver variable that implicitly assumes sectors > are 512 bytes in length more visible to the user space > seems like a step in the wrong direction. > > Alternatively the SG_GET_RESERVED_SIZE ioctl could be > modified to yield no more than max_sectors*512 . There should be one single ioctl which can be applied uniformly to all CD-type devices (in fact, to all devices using a request_queue) to learn max_sectors. This rules out using SG_GET_RESERVED_SIZE. Furthermore, if you changed SG_GET_RESERVED_SIZE in this way you would only increase the confusion. The reserved size isn't directly related to the maximum allowed DMA length, and there's no point pretending it is. What if it turns out that the reserved size is smaller than max_sectors? Then you'd force user programs to do I/O in chunks that were smaller than necessary. > >> + /* block ioctls first */ > > Why first?? I don't know -- James will have to answer. I don't see that it makes any real difference; the new code can easily be moved to the end. > That would allow the block layer to overtake any currently > defined and working sg ioctl. > Surely, if it was put in, it should be in the default case. Would the patch be okay with you if I rework it to put the new ioctls under the default case in sg? Alan Stern - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Block layer: separate out queue-oriented ioctls
Douglas Gilbert <[EMAIL PROTECTED]> wrote: > Jens Axboe wrote: > > On Fri, Feb 16 2007, Alan Stern wrote: > >> From: James Bottomley <[EMAIL PROTECTED]> > >> > >> This patch (as854) separates out the two queue-oriented ioctls from > >> the rest of the block-layer ioctls. The idea is that they should > >> apply to any driver using a request_queue, even if the driver doesn't > >> implement a block-device interface. The prototypical example is the > >> sg driver, to which the patch adds the new interface. > >> > >> This will make it possible for cdrecord and related programs to > >> retrieve reliably the max_sectors value, regardless of whether the > >> user points it to an sr or an sg device. In particular, this will > >> resolve Bugzilla entry #7026. > > > > The block bits are fine with me, the sg calling point is a bit of a sore > > thumb (a char driver calling into block layer ioctls) though. So the > > block layer bits are certainly ok with me, if Doug acks the sg bit I'll > > merge everything up. > > > > (patch left below) > > Does this need to be in the sg driver? > > What is the hardware sector size of a SES or OSD device? > > As for the max_sector variable, wouldn't it be better > to generate a new ioctl that yielded the limit in bytes? > Making a driver variable that implicitly assumes sectors > are 512 bytes in length more visible to the user space > seems like a step in the wrong direction. This is what I did propose. I know of no SCSI device made since 1986 that has a "hardware sector size". This is really a DMA size limit in bytes and if you return the number in an unrelated multiple of a fraction, you will not be able to use the optmium max transfer size. > Alternatively the SG_GET_RESERVED_SIZE ioctl could be > modified to yield no more than max_sectors*512 . This is what I did propose 3 months ago and already 2 years ago. Jörg -- EMail:[EMAIL PROTECTED] (home) Jörg Schilling D-13353 Berlin [EMAIL PROTECTED](uni) [EMAIL PROTECTED] (work) Blog: http://schily.blogspot.com/ URL: http://cdrecord.berlios.de/old/private/ ftp://ftp.berlios.de/pub/schily - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Block layer: separate out queue-oriented ioctls
Jens Axboe wrote:
> On Fri, Feb 16 2007, Alan Stern wrote:
>> From: James Bottomley <[EMAIL PROTECTED]>
>>
>> This patch (as854) separates out the two queue-oriented ioctls from
>> the rest of the block-layer ioctls. The idea is that they should
>> apply to any driver using a request_queue, even if the driver doesn't
>> implement a block-device interface. The prototypical example is the
>> sg driver, to which the patch adds the new interface.
>>
>> This will make it possible for cdrecord and related programs to
>> retrieve reliably the max_sectors value, regardless of whether the
>> user points it to an sr or an sg device. In particular, this will
>> resolve Bugzilla entry #7026.
>
> The block bits are fine with me, the sg calling point is a bit of a sore
> thumb (a char driver calling into block layer ioctls) though. So the
> block layer bits are certainly ok with me, if Doug acks the sg bit I'll
> merge everything up.
>
> (patch left below)
Does this need to be in the sg driver?
What is the hardware sector size of a SES or OSD device?
As for the max_sector variable, wouldn't it be better
to generate a new ioctl that yielded the limit in bytes?
Making a driver variable that implicitly assumes sectors
are 512 bytes in length more visible to the user space
seems like a step in the wrong direction.
Alternatively the SG_GET_RESERVED_SIZE ioctl could be
modified to yield no more than max_sectors*512 .
Doug Gilbert
P.S. See comment below
>> Signed-off-by: Alan Stern <[EMAIL PROTECTED]>
>>
>> ---
>>
>> Jens:
>>
>> James said that he feels you should be be the person to accept this
>> patch, since it affects the block layer. Please merge it and send it
>> on up the hierarchy.
>>
>> Alan Stern
>>
>>
>>
>> diff --git a/block/ioctl.c b/block/ioctl.c
>> index 58aab63..8444d0c 100644
>> --- a/block/ioctl.c
>> +++ b/block/ioctl.c
>> @@ -135,6 +135,31 @@ static int put_u64(unsigned long arg, u6
>> return put_user(val, (u64 __user *)arg);
>> }
>>
>> +static int blk_queue_locked_ioctl(struct request_queue *queue,
>> + unsigned cmd, unsigned long arg)
>> +{
>> +switch (cmd) {
>> +case BLKSSZGET: /* get block device hardware sector size */
>> +return put_int(arg, queue_hardsect_size(queue));
>> +case BLKSECTGET:
>> +return put_ushort(arg, queue->max_sectors);
>> +}
>> +return -ENOIOCTLCMD;
>> +}
>> +
>> +int blk_queue_ioctl(struct request_queue *queue, unsigned cmd,
>> +unsigned long arg)
>> +{
>> +int ret;
>> +
>> +lock_kernel();
>> +ret = blk_queue_locked_ioctl(queue, cmd, arg);
>> +unlock_kernel();
>> +
>> +return ret;
>> +}
>> +EXPORT_SYMBOL(blk_queue_ioctl);
>> +
>> static int blkdev_locked_ioctl(struct file *file, struct block_device *bdev,
>> unsigned cmd, unsigned long arg)
>> {
>> @@ -154,10 +179,6 @@ static int blkdev_locked_ioctl(struct fi
>> return put_int(arg, bdev_read_only(bdev) != 0);
>> case BLKBSZGET: /* get the logical block size (cf. BLKSSZGET) */
>> return put_int(arg, block_size(bdev));
>> -case BLKSSZGET: /* get block device hardware sector size */
>> -return put_int(arg, bdev_hardsect_size(bdev));
>> -case BLKSECTGET:
>> -return put_ushort(arg, bdev_get_queue(bdev)->max_sectors);
>> case BLKRASET:
>> case BLKFRASET:
>> if(!capable(CAP_SYS_ADMIN))
>> @@ -278,6 +299,8 @@ int blkdev_ioctl(struct inode *inode, st
>>
>> lock_kernel();
>> ret = blkdev_locked_ioctl(file, bdev, cmd, arg);
>> +if (ret == -ENOIOCTLCMD)
>> +ret = blk_queue_locked_ioctl(bdev_get_queue(bdev), cmd, arg);
>> unlock_kernel();
>> if (ret != -ENOIOCTLCMD)
>> return ret;
>> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
>> index 81e3bc7..d97244b 100644
>> --- a/drivers/scsi/sg.c
>> +++ b/drivers/scsi/sg.c
>> @@ -786,6 +786,11 @@ sg_ioctl(struct inode *inode, struct fil
>> sdp->disk->disk_name, (int) cmd_in));
>> read_only = (O_RDWR != (filp->f_flags & O_ACCMODE));
>>
>> +/* block ioctls first */
Why first??
That would allow the block layer to overtake any currently
defined and working sg ioctl.
Surely, if it was put in, it should be in the default case.
>> +result = blk_queue_ioctl(sdp->device->request_queue, cmd_in, arg);
>> +if (result != -ENOIOCTLCMD)
>> +return result;
>> +
>> switch (cmd_in) {
>> case SG_IO:
>> {
>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>> index e1c7286..550b04a 100644
>> --- a/include/linux/blkdev.h
>> +++ b/include/linux/blkdev.h
>> @@ -754,6 +754,8 @@ extern void blk_queue_prep_rq(request_qu
>> extern void blk_queue_merge_bvec(request_queue_t *, merge_bvec_fn *);
>> extern void blk_queue_dma_alignment(request_queue_t *, int);
>> extern void blk_queue_softirq_done(request_queue_t *, softirq_done_fn *);
Re: [PATCH] Block layer: separate out queue-oriented ioctls
Jens Axboe <[EMAIL PROTECTED]> wrote: > > This will make it possible for cdrecord and related programs to > > retrieve reliably the max_sectors value, regardless of whether the > > user points it to an sr or an sg device. In particular, this will > > resolve Bugzilla entry #7026. > > The block bits are fine with me, the sg calling point is a bit of a sore > thumb (a char driver calling into block layer ioctls) though. So the > block layer bits are certainly ok with me, if Doug acks the sg bit I'll > merge everything up. If you care about this kind of order, you would first need to eliminate the ability of doing low level things like SG_IO from block drivers as this is similar low level stuff that rather belongs to low level drivers. Any driver that allows to use low level interfaces to send RAW SCSI commands also needs access to other lowlevel stuff like the ability to know the max. DMA size. Jörg -- EMail:[EMAIL PROTECTED] (home) Jörg Schilling D-13353 Berlin [EMAIL PROTECTED](uni) [EMAIL PROTECTED] (work) Blog: http://schily.blogspot.com/ URL: http://cdrecord.berlios.de/old/private/ ftp://ftp.berlios.de/pub/schily - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Block layer: separate out queue-oriented ioctls
On Fri, Feb 16 2007, Alan Stern wrote:
> From: James Bottomley <[EMAIL PROTECTED]>
>
> This patch (as854) separates out the two queue-oriented ioctls from
> the rest of the block-layer ioctls. The idea is that they should
> apply to any driver using a request_queue, even if the driver doesn't
> implement a block-device interface. The prototypical example is the
> sg driver, to which the patch adds the new interface.
>
> This will make it possible for cdrecord and related programs to
> retrieve reliably the max_sectors value, regardless of whether the
> user points it to an sr or an sg device. In particular, this will
> resolve Bugzilla entry #7026.
The block bits are fine with me, the sg calling point is a bit of a sore
thumb (a char driver calling into block layer ioctls) though. So the
block layer bits are certainly ok with me, if Doug acks the sg bit I'll
merge everything up.
(patch left below)
> Signed-off-by: Alan Stern <[EMAIL PROTECTED]>
>
> ---
>
> Jens:
>
> James said that he feels you should be be the person to accept this
> patch, since it affects the block layer. Please merge it and send it
> on up the hierarchy.
>
> Alan Stern
>
>
>
> diff --git a/block/ioctl.c b/block/ioctl.c
> index 58aab63..8444d0c 100644
> --- a/block/ioctl.c
> +++ b/block/ioctl.c
> @@ -135,6 +135,31 @@ static int put_u64(unsigned long arg, u6
> return put_user(val, (u64 __user *)arg);
> }
>
> +static int blk_queue_locked_ioctl(struct request_queue *queue,
> + unsigned cmd, unsigned long arg)
> +{
> + switch (cmd) {
> + case BLKSSZGET: /* get block device hardware sector size */
> + return put_int(arg, queue_hardsect_size(queue));
> + case BLKSECTGET:
> + return put_ushort(arg, queue->max_sectors);
> + }
> + return -ENOIOCTLCMD;
> +}
> +
> +int blk_queue_ioctl(struct request_queue *queue, unsigned cmd,
> + unsigned long arg)
> +{
> + int ret;
> +
> + lock_kernel();
> + ret = blk_queue_locked_ioctl(queue, cmd, arg);
> + unlock_kernel();
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(blk_queue_ioctl);
> +
> static int blkdev_locked_ioctl(struct file *file, struct block_device *bdev,
> unsigned cmd, unsigned long arg)
> {
> @@ -154,10 +179,6 @@ static int blkdev_locked_ioctl(struct fi
> return put_int(arg, bdev_read_only(bdev) != 0);
> case BLKBSZGET: /* get the logical block size (cf. BLKSSZGET) */
> return put_int(arg, block_size(bdev));
> - case BLKSSZGET: /* get block device hardware sector size */
> - return put_int(arg, bdev_hardsect_size(bdev));
> - case BLKSECTGET:
> - return put_ushort(arg, bdev_get_queue(bdev)->max_sectors);
> case BLKRASET:
> case BLKFRASET:
> if(!capable(CAP_SYS_ADMIN))
> @@ -278,6 +299,8 @@ int blkdev_ioctl(struct inode *inode, st
>
> lock_kernel();
> ret = blkdev_locked_ioctl(file, bdev, cmd, arg);
> + if (ret == -ENOIOCTLCMD)
> + ret = blk_queue_locked_ioctl(bdev_get_queue(bdev), cmd, arg);
> unlock_kernel();
> if (ret != -ENOIOCTLCMD)
> return ret;
> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
> index 81e3bc7..d97244b 100644
> --- a/drivers/scsi/sg.c
> +++ b/drivers/scsi/sg.c
> @@ -786,6 +786,11 @@ sg_ioctl(struct inode *inode, struct fil
> sdp->disk->disk_name, (int) cmd_in));
> read_only = (O_RDWR != (filp->f_flags & O_ACCMODE));
>
> + /* block ioctls first */
> + result = blk_queue_ioctl(sdp->device->request_queue, cmd_in, arg);
> + if (result != -ENOIOCTLCMD)
> + return result;
> +
> switch (cmd_in) {
> case SG_IO:
> {
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index e1c7286..550b04a 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -754,6 +754,8 @@ extern void blk_queue_prep_rq(request_qu
> extern void blk_queue_merge_bvec(request_queue_t *, merge_bvec_fn *);
> extern void blk_queue_dma_alignment(request_queue_t *, int);
> extern void blk_queue_softirq_done(request_queue_t *, softirq_done_fn *);
> +extern int blk_queue_ioctl(struct request_queue *queue, unsigned cmd,
> +unsigned long arg);
> extern struct backing_dev_info *blk_get_backing_dev_info(struct block_device
> *bdev);
> extern int blk_queue_ordered(request_queue_t *, unsigned, prepare_flush_fn
> *);
> extern void blk_queue_issue_flush_fn(request_queue_t *, issue_flush_fn *);
>
--
Jens Axboe
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
