Re: [PATCH 1/9] blk-sysfs: Add 'chunk_sectors' to sysfs attributes

From: Hannes Reinecke <hare@suse.de>
To: Bart Van Assche <bart.vanassche@sandisk.com>, Jens Axboe <axboe@fb.com>
Cc: linux-block@vger.kernel.org,
	"Martin K. Petersen" <martin.petersen@oracle.com>,
	Christoph Hellwig <hch@lst.de>,
	Shaun Tancheff <shaun.tancheff@seagate.com>,
	Damien Le Moal <damien.lemoal@hgst.com>,
	linux-scsi@vger.kernel.org,
	Sathya Prakash <sathya.prakash@broadcom.com>
Subject: Re: [PATCH 1/9] blk-sysfs: Add 'chunk_sectors' to sysfs attributes
Date: Fri, 15 Apr 2016 08:01:11 +0200	[thread overview]
Message-ID: <571083A7.80009@suse.de> (raw)
In-Reply-To: <570FEAF8.7060606@sandisk.com>

On 04/14/2016 09:09 PM, Bart Van Assche wrote:
> On 04/04/2016 03:00 AM, Hannes Reinecke wrote:
>> The queue limits already have a 'chunk_sectors' setting, so
>> we should be presenting it via sysfs.
> 
> This patch does more than exporting chunk_sectors via sysfs. It also
> makes that parameter configurable. Please mention this in the patch
> description.
> 
> My understanding of the block drivers that call
> blk_queue_chunk_sectors() is that increasing the chunk_sectors
> parameter will break these drivers. I think the single
> queue_limits.chunk_sectors parameter needs to be converted into two
> parameters:
> - The value set by the block driver by calling
>   blk_queue_chunk_sectors().
> - The value configured from user space through sysfs.
> 
> This will allow to ensure that the chunk_sectors parameter can be
> increased from user space and also that it cannot be decreased.
> 
>> +static ssize_t
>> +queue_chunk_sectors_store(struct request_queue *q, const char
>> *page, size_t count)
>> +{
>> +    unsigned long chunk_sectors;
>> +
>> +    ssize_t ret = queue_var_store(&chunk_sectors, page, count);
>> +    if (ret < 0)
>> +        return ret;
>> +    spin_lock_irq(q->queue_lock);
>> +    blk_queue_chunk_sectors(q, chunk_sectors);
>> +    spin_unlock_irq(q->queue_lock);
>> +
>> +    return ret;
>> +}
> 
> In blk_queue_chunk_sectors() I found the following:
> 
>     BUG_ON(!is_power_of_2(chunk_sectors));
> 
> Please make sure that this BUG_ON() cannot be triggered from user
> space.
> 
> Additionally, an update for Documentation/block/queue-sysfs.txt is
> missing from this patch.
> 

Ah, right.
Actually, making the sysfs attribute writeable is a leftover from
the original (debugging) patchset, so I'll be removing it from the
next iteration of this patchset.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html