Message-ID: <20070429181646.GB3551@infradead.org>
Date:	Sun, 29 Apr 2007 19:16:46 +0100
From:	Christoph Hellwig <hch@...radead.org>
To:	Jeremy Fitzhardinge <jeremy@...p.org>
Cc:	Andi Kleen <ak@...e.de>, Andrew Morton <akpm@...ux-foundation.org>,
	virtualization@...ts.osdl.org, lkml <linux-kernel@...r.kernel.org>,
	Chris Wright <chrisw@...s-sol.org>,
	Ian Pratt <ian.pratt@...source.com>,
	Christian Limpach <Christian.Limpach@...cam.ac.uk>,
	Arjan van de Ven <arjan@...radead.org>,
	Greg KH <greg@...ah.com>, Jens Axboe <axboe@...nel.dk>
Subject: Re: [patch 26/32] xen: Add Xen virtual block device driver.

>  source "drivers/s390/block/Kconfig"
> +source "drivers/block/xen/Kconfig"

Please don't add a new subdirectory for a tiny new driver that
really should be only a single .c file.

> +config XEN_BLKDEV_FRONTEND
> +	tristate "Block device frontend driver"
> +	depends on XEN
> +	default y

Please kill the default statement.  We really should have script that
autorejects every new driver that wants to be default y..

> +#include <linux/version.h>

not needed, as usual.

> +#define BLKIF_STATE_DISCONNECTED 0
> +#define BLKIF_STATE_CONNECTED    1
> +#define BLKIF_STATE_SUSPENDED    2

enum, please.

> +static void connect(struct blkfront_info *);
> +static void blkfront_closing(struct xenbus_device *);
> +static int blkfront_remove(struct xenbus_device *);
> +static int talk_to_backend(struct xenbus_device *, struct blkfront_info *);
> +static int setup_blkring(struct xenbus_device *, struct blkfront_info *);
> +
> +static void kick_pending_request_queues(struct blkfront_info *);
> +
> +static irqreturn_t blkif_int(int irq, void *dev_id);
> +static void blkif_restart_queue(struct work_struct *work);
> +static void blkif_recover(struct blkfront_info *);
> +static void blkif_completion(struct blk_shadow *);
> +static void blkif_free(struct blkfront_info *, int);

Please get rid of all the forward-declarations by putting the code
into proper order.

> +static inline int GET_ID_FROM_FREELIST(
> +	struct blkfront_info *info)
> +{
> +	unsigned long free = info->shadow_free;
> +	BUG_ON(free > BLK_RING_SIZE);
> +	info->shadow_free = info->shadow[free].req.id;
> +	info->shadow[free].req.id = 0x0fffffee; /* debug */
> +	return free;
> +}
> +
> +static inline void ADD_ID_TO_FREELIST(
> +	struct blkfront_info *info, unsigned long id)
> +{
> +	info->shadow[id].req.id  = info->shadow_free;
> +	info->shadow[id].request = 0;
> +	info->shadow_free = id;
> +}

Please give these proper lowercased names, and while you're at it
please kill all the inline statements you have and let the compiler
do it's work.


> +static irqreturn_t blkif_int(int irq, void *dev_id)

Please call interrupt handlers foo_interrupt, that makes peopl see what's
going on much more easily.

> +static void blkif_recover(struct blkfront_info *info)
> +{
> +	int i;
> +	struct blkif_request *req;
> +	struct blk_shadow *copy;
> +	int j;
> +
> +	/* Stage 1: Make a safe copy of the shadow state. */
> +	copy = kmalloc(sizeof(info->shadow), GFP_KERNEL | __GFP_NOFAIL);

Please don't use __GFP_NOFAIL in any new code.

> +/* ** Driver Registration ** */

No one would have guesses without that comments..

> +extern int blkif_ioctl(struct inode *inode, struct file *filep,
> +		       unsigned command, unsigned long argument);

doesn't actually exist anywhere.

> +extern int blkif_check(dev_t dev);

ditto

> +extern int blkif_revalidate(dev_t dev);

ditto.

> +/******************************************************************************

Just kill these *, okay?

> + * vbd.c

Please don't put filenames in the top of file comments, they'll get out
of date far too soon while serving no purpose at all.

> +#define BLKIF_MAJOR(dev) ((dev)>>8)
> +#define BLKIF_MINOR(dev) ((dev) & 0xff)

Please make these proper xenblk-prefixed inlines in lower case.

> +
> +static struct xlbd_type_info xvd_type_info = {
> +	.partn_shift = 4,
> +	.disks_per_major = 16,
> +	.devname = "xvd",
> +	.diskname = "xvd"
> +};
> +
> +static struct xlbd_major_info xvd_major_info = {
> +	.major = XENVBD_MAJOR,
> +	.type = &xvd_type_info
> +};

The structures seem quite useless and the code would be much
cleaner without them.  What's the reason for their existance?

> +
> +/* Information about our VBDs. */
> +#define MAX_VBDS 64
> +static LIST_HEAD(vbds_list);
> +
> +static struct block_device_operations xlvbd_block_fops =
> +{
> +	.owner = THIS_MODULE,
> +	.open = blkif_open,
> +	.release = blkif_release,
> +	.getgeo = blkif_getgeo
> +};
> +
> +DEFINE_SPINLOCK(blkif_io_lock);
> +
> +int
> +xlvbd_alloc_major(void)
> +{
> +	if (register_blkdev(xvd_major_info.major,
> +			    xvd_major_info.type->devname)) {
> +		printk(KERN_WARNING "xen_blk: can't get major %d with name %s\n",
> +		       xvd_major_info.major, xvd_major_info.type->devname);
> +		return -1;
> +	}
> +	return 0;
> +}

Useless wrapper, please just kill it.

> +
> +static int
> +xlvbd_init_blk_queue(struct gendisk *gd, u16 sector_size)
> +{
> +	request_queue_t *rq;
> +
> +	rq = blk_init_queue(do_blkif_request, &blkif_io_lock);
> +	if (rq == NULL)
> +		return -1;
> +
> +	elevator_init(rq, "noop");
> +
> +	/* Hard sector size and max sectors impersonate the equiv. hardware. */
> +	blk_queue_hardsect_size(rq, sector_size);
> +	blk_queue_max_sectors(rq, 512);
> +
> +	/* Each segment in a request is up to an aligned page in size. */
> +	blk_queue_segment_boundary(rq, PAGE_SIZE - 1);
> +	blk_queue_max_segment_size(rq, PAGE_SIZE);
> +
> +	/* Ensure a merged request will fit in a single I/O ring slot. */
> +	blk_queue_max_phys_segments(rq, BLKIF_MAX_SEGMENTS_PER_REQUEST);
> +	blk_queue_max_hw_segments(rq, BLKIF_MAX_SEGMENTS_PER_REQUEST);
> +
> +	/* Make sure buffer addresses are sector-aligned. */
> +	blk_queue_dma_alignment(rq, 511);
> +
> +	gd->queue = rq;
> +
> +	return 0;

Useless wrapper, please just kill it.

> +static int
> +xlvbd_alloc_gendisk(int minor, blkif_sector_t capacity, int vdevice,
> +		    u16 vdisk_info, u16 sector_size,
> +		    struct blkfront_info *info)
> +{
> +	struct gendisk *gd;
> +	struct xlbd_major_info *mi;
> +	int nr_minors = 1;
> +	int err = -ENODEV;
> +
> +	BUG_ON(info->gd != NULL);
> +	BUG_ON(info->mi != NULL);
> +	BUG_ON(info->rq != NULL);
> +
> +	mi = &xvd_major_info;
> +	info->mi = mi;
> +
> +	if ((minor & ((1 << mi->type->partn_shift) - 1)) == 0)
> +		nr_minors = 1 << mi->type->partn_shift;
> +
> +	gd = alloc_disk(nr_minors);
> +	if (gd == NULL)
> +		goto out;
> +
> +	if (nr_minors > 1)
> +		sprintf(gd->disk_name, "%s%c", mi->type->diskname,
> +			'a' + mi->index * mi->type->disks_per_major +
> +			(minor >> mi->type->partn_shift));
> +	else
> +		sprintf(gd->disk_name, "%s%c%d", mi->type->diskname,
> +			'a' + mi->index * mi->type->disks_per_major +
> +			(minor >> mi->type->partn_shift),
> +			minor & ((1 << mi->type->partn_shift) - 1));
> +
> +	gd->major = mi->major;
> +	gd->first_minor = minor;
> +	gd->fops = &xlvbd_block_fops;
> +	gd->private_data = info;
> +	gd->driverfs_dev = &(info->xbdev->dev);
> +	set_capacity(gd, capacity);
> +
> +	if (xlvbd_init_blk_queue(gd, sector_size)) {
> +		del_gendisk(gd);
> +		goto out;
> +	}
> +
> +	info->rq = gd->queue;
> +	info->gd = gd;
> +
> +	if (info->feature_barrier)
> +		xlvbd_barrier(info);
> +
> +	if (vdisk_info & VDISK_READONLY)
> +		set_disk_ro(gd, 1);
> +
> +	if (vdisk_info & VDISK_REMOVABLE)
> +		gd->flags |= GENHD_FL_REMOVABLE;
> +
> +	if (vdisk_info & VDISK_CDROM)
> +		gd->flags |= GENHD_FL_CD;
> +
> +	return 0;
> +
> + out:
> +	info->mi = NULL;
> +	return err;
> +}
> +
> +int
> +xlvbd_add(blkif_sector_t capacity, int vdevice, u16 vdisk_info,
> +	  u16 sector_size, struct blkfront_info *info)
> +{
> +	struct block_device *bd;
> +	int err = 0;
> +
> +	info->dev = MKDEV(BLKIF_MAJOR(vdevice), BLKIF_MINOR(vdevice));
> +
> +	bd = bdget(info->dev);
> +	if (bd == NULL)
> +		return -ENODEV;
> +
> +	err = xlvbd_alloc_gendisk(BLKIF_MINOR(vdevice), capacity, vdevice,
> +				  vdisk_info, sector_size, info);
> +
> +	bdput(bd);
> +	return err;
> +}

Please just inline this in the caller, and get rid of the bdget mess.

> +void
> +xlvbd_del(struct blkfront_info *info)
> +{
> +	if (info->mi == NULL)
> +		return;
> +
> +	BUG_ON(info->gd == NULL);
> +	del_gendisk(info->gd);
> +	put_disk(info->gd);
> +	info->gd = NULL;
> +
> +	info->mi = NULL;
> +
> +	BUG_ON(info->rq == NULL);
> +	blk_cleanup_queue(info->rq);
> +	info->rq = NULL;
> +}

Note that the del_gendisk should happen before actual
cleanup happens on the device, so this function should
go away and the only caller should do things in the proper order.

> +int
> +xlvbd_barrier(struct blkfront_info *info)
> +{
> +	int err;
> +
> +	err = blk_queue_ordered(info->rq,
> +		info->feature_barrier ? QUEUE_ORDERED_DRAIN : QUEUE_ORDERED_NONE, NULL);
> +	if (err)
> +		return err;
> +	printk("blkfront: %s: barriers %s\n",
> +	       info->gd->disk_name, info->feature_barrier ? "enabled" : "disabled");
> +	return 0;
> +}
i
Please kill this rather useless wrapper.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists