Re: [PATCH 2/2] Add BLKGETQUEUEINFO for reading block queue attributes

[Arnd Bergmann <arnd@arndb.de>]

On Thursday 09 December 2010 16:25:37 Lukas Czerner wrote:
> +enum blkq_info_type {
> +       BLKQ_REQUESTS = 0,
> +       BLKQ_RA,
> +       BLKQ_MAX_HW_SECTORS,
> +       BLKQ_MAX_SECTORS,
> +       BLKQ_MAX_SEGMENTS,
> +       BLKQ_MAX_INTEGRITY_SEGMENTS,
> +       BLKQ_MAX_SEGMENT_SIZE,
> +       BLKQ_IOSCHED,
> +       BLKQ_HW_SECTOR_SIZE,
> +       BLKQ_LOGICAL_BLOCK_SIZE,
> +       BLKQ_PHYSICAL_BLOCK_SIZE,
> +       BLKQ_IO_MIN,
> +       BLKQ_IO_OPT,
> +       BLKQ_DISCARD_GRANULARITY,
> +       BLKQ_DISCARD_MAX,
> +       BLKQ_DISCARD_ZEROES_DATA,
> +       BLKQ_NONROT,
> +       BLKQ_NOMERGES,
> +       BLKQ_RQ_AFFINITY,
> +       BLKQ_IOSTATS,
> +       BLKQ_RANDOM,
> +       BLKQ_END,               /* Last item = quantity of items */
> +};
> +
> +struct blk_queue_info {
> +       enum blkq_info_type type;
> +       unsigned long data;
> +};

You are adding another indirection to an indirect system call
here. Besides the problems that Greg mentioned, this is
also really ugly. If it turns out that we want an ioctl interface
for this after all, better make it either one command per value,
or one data structure that contains all the values (plus some
reserved fields for future extensions).

Furthermore, you should use neither enum nor long data types
in a data structure that is used as an ABI. Use either __u32
or __u64 here, and make sure you have no padding in the middle
or at the end if you mix the two.

> diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
> index 30b8815..71bd06c 100644
> --- a/include/linux/sysfs.h
> +++ b/include/linux/sysfs.h
> @@ -112,6 +112,7 @@ struct bin_attribute {
>  struct sysfs_ops {
>         ssize_t (*show)(struct kobject *, struct attribute *,char *);
>         ssize_t (*store)(struct kobject *,struct attribute *,const char *, size_t);
> +       ssize_t (*get)(struct kobject *, struct attribute *, void *);
>  };
>  

This looks like it can significantly add to the .data size of the kernel.

	Arnd