LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Douglas Gilbert <dougg@torque.net>
To: Jens Axboe <jens.axboe@oracle.com>
Cc: Alan Stern <stern@rowland.harvard.edu>,
	James Bottomley <James.Bottomley@SteelEye.com>,
	Joerg Schilling <Joerg.Schilling@fokus.fraunhofer.de>,
	Kernel development list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] Block layer: separate out queue-oriented ioctls
Date: Sat, 17 Feb 2007 22:43:02 -0500	[thread overview]
Message-ID: <45D7CB46.6060807@torque.net> (raw)
In-Reply-To: <20070217062832.GE3689@kernel.dk>

Jens Axboe wrote:
> On Fri, Feb 16 2007, Alan Stern wrote:
>> From: James Bottomley <James.Bottomley@SteelEye.com>
>>
>> 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 <stern@rowland.harvard.edu>
>>
>> ---
>>
>> 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 *);
>> +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 *);
>>
> 


  parent reply	other threads:[~2007-02-18  3:43 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-02-16 19:37 Alan Stern
2007-02-17  6:28 ` Jens Axboe
2007-02-17 21:18   ` Joerg Schilling
2007-02-18  3:43   ` Douglas Gilbert [this message]
2007-02-18 12:37     ` Joerg Schilling
2007-02-18 16:44     ` Alan Stern
2007-02-18 18:27       ` Joerg Schilling
2007-02-19 16:06         ` Alan Stern
2007-02-19 16:08           ` Joerg Schilling
2007-02-19 17:06             ` Alan Stern
2007-02-19 22:25               ` Douglas Gilbert
2007-02-20  3:48                 ` Alan Stern
2007-02-20  4:47                   ` Douglas Gilbert
2007-02-20 15:55                     ` Alan Stern
2007-04-26  9:19                 ` Joerg Schilling
2007-04-26 15:04                   ` Alan Stern
2007-04-26 15:08                     ` James Bottomley

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=45D7CB46.6060807@torque.net \
    --to=dougg@torque.net \
    --cc=James.Bottomley@SteelEye.com \
    --cc=Joerg.Schilling@fokus.fraunhofer.de \
    --cc=jens.axboe@oracle.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=stern@rowland.harvard.edu \
    --subject='Re: [PATCH] Block layer: separate out queue-oriented ioctls' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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).