* [PATCH 1/8] block: kill genhd_media_change_notify()
2010-12-08 19:57 [PATCHSET] block/SCSI: implement in-kernel disk event handling, take#2 Tejun Heo
@ 2010-12-08 19:57 ` Tejun Heo
2010-12-08 19:57 ` [PATCH 2/8] block: move register_disk() and del_gendisk() to block/genhd.c Tejun Heo
` (7 subsequent siblings)
8 siblings, 0 replies; 33+ messages in thread
From: Tejun Heo @ 2010-12-08 19:57 UTC (permalink / raw)
To: jeff, linux-ide, axboe, linux-kernel, linux-scsi, kay.sievers,
jack, James.Bottomley
Cc: Tejun Heo
There's no user of the facility. Kill it.
Signed-off-by: Tejun Heo <tj@kernel.org>
---
block/genhd.c | 25 -------------------------
include/linux/genhd.h | 1 -
2 files changed, 0 insertions(+), 26 deletions(-)
diff --git a/block/genhd.c b/block/genhd.c
index 5fa2b44..0905ab2 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1110,29 +1110,6 @@ static int __init proc_genhd_init(void)
module_init(proc_genhd_init);
#endif /* CONFIG_PROC_FS */
-static void media_change_notify_thread(struct work_struct *work)
-{
- struct gendisk *gd = container_of(work, struct gendisk, async_notify);
- char event[] = "MEDIA_CHANGE=1";
- char *envp[] = { event, NULL };
-
- /*
- * set enviroment vars to indicate which event this is for
- * so that user space will know to go check the media status.
- */
- kobject_uevent_env(&disk_to_dev(gd)->kobj, KOBJ_CHANGE, envp);
- put_device(gd->driverfs_dev);
-}
-
-#if 0
-void genhd_media_change_notify(struct gendisk *disk)
-{
- get_device(disk->driverfs_dev);
- schedule_work(&disk->async_notify);
-}
-EXPORT_SYMBOL_GPL(genhd_media_change_notify);
-#endif /* 0 */
-
dev_t blk_lookup_devt(const char *name, int partno)
{
dev_t devt = MKDEV(0, 0);
@@ -1198,8 +1175,6 @@ struct gendisk *alloc_disk_node(int minors, int node_id)
disk_to_dev(disk)->class = &block_class;
disk_to_dev(disk)->type = &disk_type;
device_initialize(disk_to_dev(disk));
- INIT_WORK(&disk->async_notify,
- media_change_notify_thread);
}
return disk;
}
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 7a7b9c1..5e4e692 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -173,7 +173,6 @@ struct gendisk {
struct timer_rand_state *random;
atomic_t sync_io; /* RAID */
- struct work_struct async_notify;
#ifdef CONFIG_BLK_DEV_INTEGRITY
struct blk_integrity *integrity;
#endif
--
1.7.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH 2/8] block: move register_disk() and del_gendisk() to block/genhd.c
2010-12-08 19:57 [PATCHSET] block/SCSI: implement in-kernel disk event handling, take#2 Tejun Heo
2010-12-08 19:57 ` [PATCH 1/8] block: kill genhd_media_change_notify() Tejun Heo
@ 2010-12-08 19:57 ` Tejun Heo
2010-12-08 19:57 ` [PATCH 3/8] implement in-kernel gendisk events handling Tejun Heo
` (6 subsequent siblings)
8 siblings, 0 replies; 33+ messages in thread
From: Tejun Heo @ 2010-12-08 19:57 UTC (permalink / raw)
To: jeff, linux-ide, axboe, linux-kernel, linux-scsi, kay.sievers,
jack, James.Bottomley
Cc: Tejun Heo
There's no reason for register_disk() and del_gendisk() to be in
fs/partitions/check.c. Move both to genhd.c. While at it, collapse
unlink_gendisk(), which was artificially in a separate function due to
genhd.c / check.c split, into del_gendisk().
Signed-off-by: Tejun Heo <tj@kernel.org>
---
block/genhd.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++--
fs/partitions/check.c | 89 -----------------------------------------------
include/linux/blkdev.h | 1 -
include/linux/genhd.h | 1 -
4 files changed, 87 insertions(+), 94 deletions(-)
diff --git a/block/genhd.c b/block/genhd.c
index 0905ab2..2e5e4c0 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -502,6 +502,64 @@ static int exact_lock(dev_t devt, void *data)
return 0;
}
+void register_disk(struct gendisk *disk)
+{
+ struct device *ddev = disk_to_dev(disk);
+ struct block_device *bdev;
+ struct disk_part_iter piter;
+ struct hd_struct *part;
+ int err;
+
+ ddev->parent = disk->driverfs_dev;
+
+ dev_set_name(ddev, disk->disk_name);
+
+ /* delay uevents, until we scanned partition table */
+ dev_set_uevent_suppress(ddev, 1);
+
+ if (device_add(ddev))
+ return;
+ if (!sysfs_deprecated) {
+ err = sysfs_create_link(block_depr, &ddev->kobj,
+ kobject_name(&ddev->kobj));
+ if (err) {
+ device_del(ddev);
+ return;
+ }
+ }
+ disk->part0.holder_dir = kobject_create_and_add("holders", &ddev->kobj);
+ disk->slave_dir = kobject_create_and_add("slaves", &ddev->kobj);
+
+ /* No minors to use for partitions */
+ if (!disk_partitionable(disk))
+ goto exit;
+
+ /* No such device (e.g., media were just removed) */
+ if (!get_capacity(disk))
+ goto exit;
+
+ bdev = bdget_disk(disk, 0);
+ if (!bdev)
+ goto exit;
+
+ bdev->bd_invalidated = 1;
+ err = blkdev_get(bdev, FMODE_READ, NULL);
+ if (err < 0)
+ goto exit;
+ blkdev_put(bdev, FMODE_READ);
+
+exit:
+ /* announce disk after possible partitions are created */
+ dev_set_uevent_suppress(ddev, 0);
+ kobject_uevent(&ddev->kobj, KOBJ_ADD);
+
+ /* announce possible partitions */
+ disk_part_iter_init(&piter, disk, 0);
+ while ((part = disk_part_iter_next(&piter)))
+ kobject_uevent(&part_to_dev(part)->kobj, KOBJ_ADD);
+ disk_part_iter_exit(&piter);
+}
+
/**
* add_disk - add partitioning information to kernel list
* @disk: per-device partitioning information
@@ -552,17 +610,43 @@ void add_disk(struct gendisk *disk)
"bdi");
WARN_ON(retval);
}
-
EXPORT_SYMBOL(add_disk);
-EXPORT_SYMBOL(del_gendisk); /* in partitions/check.c */
-void unlink_gendisk(struct gendisk *disk)
+void del_gendisk(struct gendisk *disk)
{
+ struct disk_part_iter piter;
+ struct hd_struct *part;
+
+ /* invalidate stuff */
+ disk_part_iter_init(&piter, disk,
+ DISK_PITER_INCL_EMPTY | DISK_PITER_REVERSE);
+ while ((part = disk_part_iter_next(&piter))) {
+ invalidate_partition(disk, part->partno);
+ delete_partition(disk, part->partno);
+ }
+ disk_part_iter_exit(&piter);
+
+ invalidate_partition(disk, 0);
+ blk_free_devt(disk_to_dev(disk)->devt);
+ set_capacity(disk, 0);
+ disk->flags &= ~GENHD_FL_UP;
+
sysfs_remove_link(&disk_to_dev(disk)->kobj, "bdi");
bdi_unregister(&disk->queue->backing_dev_info);
blk_unregister_queue(disk);
blk_unregister_region(disk_devt(disk), disk->minors);
+
+ part_stat_set_all(&disk->part0, 0);
+ disk->part0.stamp = 0;
+
+ kobject_put(disk->part0.holder_dir);
+ kobject_put(disk->slave_dir);
+ disk->driverfs_dev = NULL;
+ if (!sysfs_deprecated)
+ sysfs_remove_link(block_depr, dev_name(disk_to_dev(disk)));
+ device_del(disk_to_dev(disk));
}
+EXPORT_SYMBOL(del_gendisk);
/**
* get_gendisk - get partitioning information for a given device
diff --git a/fs/partitions/check.c b/fs/partitions/check.c
index bdf8d3c..9a48d65 100644
--- a/fs/partitions/check.c
+++ b/fs/partitions/check.c
@@ -516,65 +516,6 @@ out_put:
return ERR_PTR(err);
}
-/* Not exported, helper to add_disk(). */
-void register_disk(struct gendisk *disk)
-{
- struct device *ddev = disk_to_dev(disk);
- struct block_device *bdev;
- struct disk_part_iter piter;
- struct hd_struct *part;
- int err;
-
- ddev->parent = disk->driverfs_dev;
-
- dev_set_name(ddev, disk->disk_name);
-
- /* delay uevents, until we scanned partition table */
- dev_set_uevent_suppress(ddev, 1);
-
- if (device_add(ddev))
- return;
- if (!sysfs_deprecated) {
- err = sysfs_create_link(block_depr, &ddev->kobj,
- kobject_name(&ddev->kobj));
- if (err) {
- device_del(ddev);
- return;
- }
- }
- disk->part0.holder_dir = kobject_create_and_add("holders", &ddev->kobj);
- disk->slave_dir = kobject_create_and_add("slaves", &ddev->kobj);
-
- /* No minors to use for partitions */
- if (!disk_partitionable(disk))
- goto exit;
-
- /* No such device (e.g., media were just removed) */
- if (!get_capacity(disk))
- goto exit;
-
- bdev = bdget_disk(disk, 0);
- if (!bdev)
- goto exit;
-
- bdev->bd_invalidated = 1;
- err = blkdev_get(bdev, FMODE_READ, NULL);
- if (err < 0)
- goto exit;
- blkdev_put(bdev, FMODE_READ);
-
-exit:
- /* announce disk after possible partitions are created */
- dev_set_uevent_suppress(ddev, 0);
- kobject_uevent(&ddev->kobj, KOBJ_ADD);
-
- /* announce possible partitions */
- disk_part_iter_init(&piter, disk, 0);
- while ((part = disk_part_iter_next(&piter)))
- kobject_uevent(&part_to_dev(part)->kobj, KOBJ_ADD);
- disk_part_iter_exit(&piter);
-}
-
static bool disk_unlock_native_capacity(struct gendisk *disk)
{
const struct block_device_operations *bdops = disk->fops;
@@ -737,33 +678,3 @@ fail:
}
EXPORT_SYMBOL(read_dev_sector);
-
-void del_gendisk(struct gendisk *disk)
-{
- struct disk_part_iter piter;
- struct hd_struct *part;
-
- /* invalidate stuff */
- disk_part_iter_init(&piter, disk,
- DISK_PITER_INCL_EMPTY | DISK_PITER_REVERSE);
- while ((part = disk_part_iter_next(&piter))) {
- invalidate_partition(disk, part->partno);
- delete_partition(disk, part->partno);
- }
- disk_part_iter_exit(&piter);
-
- invalidate_partition(disk, 0);
- blk_free_devt(disk_to_dev(disk)->devt);
- set_capacity(disk, 0);
- disk->flags &= ~GENHD_FL_UP;
- unlink_gendisk(disk);
- part_stat_set_all(&disk->part0, 0);
- disk->part0.stamp = 0;
-
- kobject_put(disk->part0.holder_dir);
- kobject_put(disk->slave_dir);
- disk->driverfs_dev = NULL;
- if (!sysfs_deprecated)
- sysfs_remove_link(block_depr, dev_name(disk_to_dev(disk)));
- device_del(disk_to_dev(disk));
-}
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index aae86fd..83031bc 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -643,7 +643,6 @@ static inline void rq_flush_dcache_pages(struct request *rq)
extern int blk_register_queue(struct gendisk *disk);
extern void blk_unregister_queue(struct gendisk *disk);
-extern void register_disk(struct gendisk *dev);
extern void generic_make_request(struct bio *bio);
extern void blk_rq_init(struct request_queue *q, struct request *rq);
extern void blk_put_request(struct request *);
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 5e4e692..56e17ed 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -394,7 +394,6 @@ extern void part_round_stats(int cpu, struct hd_struct *part);
/* block/genhd.c */
extern void add_disk(struct gendisk *disk);
extern void del_gendisk(struct gendisk *gp);
-extern void unlink_gendisk(struct gendisk *gp);
extern struct gendisk *get_gendisk(dev_t dev, int *partno);
extern struct block_device *bdget_disk(struct gendisk *disk, int partno);
--
1.7.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH 3/8] implement in-kernel gendisk events handling
2010-12-08 19:57 [PATCHSET] block/SCSI: implement in-kernel disk event handling, take#2 Tejun Heo
2010-12-08 19:57 ` [PATCH 1/8] block: kill genhd_media_change_notify() Tejun Heo
2010-12-08 19:57 ` [PATCH 2/8] block: move register_disk() and del_gendisk() to block/genhd.c Tejun Heo
@ 2010-12-08 19:57 ` Tejun Heo
2010-12-08 19:57 ` [PATCH 4/8] cdrom: add ->check_events() support Tejun Heo
` (5 subsequent siblings)
8 siblings, 0 replies; 33+ messages in thread
From: Tejun Heo @ 2010-12-08 19:57 UTC (permalink / raw)
To: jeff, linux-ide, axboe, linux-kernel, linux-scsi, kay.sievers,
jack, James.Bottomley
Cc: Tejun Heo
Currently, media presence polling for removeable block devices is done
from userland. There are several issues with this.
* Polling is done by periodically opening the device. For SCSI
devices, the command sequence generated by such action involves a
few different commands including TEST_UNIT_READY. This behavior,
while perfectly legal, is different from Windows which only issues
single command, GET_EVENT_STATUS_NOTIFICATION. Unfortunately, some
ATAPI devices lock up after being periodically queried such command
sequences.
* There is no reliable and unintrusive way for a userland program to
tell whether the target device is safe for media presence polling.
For example, polling for media presence during an on-going burning
session can make it fail. The polling program can avoid this by
opening the device with O_EXCL but then it risks making a valid
exclusive user of the device fail w/ -EBUSY.
* Userland polling is unnecessarily heavy and in-kernel implementation
is lighter and better coordinated (workqueue, timer slack).
This patch implements framework for in-kernel disk event handling,
which includes media presence polling.
* bdops->check_events() is added, which supercedes ->media_changed().
It should check whether there's any pending event and return if so.
Currently, two events are defined - DISK_EVENT_MEDIA_CHANGE and
DISK_EVENT_EJECT_REQUEST. ->check_events() is guaranteed not to be
called parallelly.
* gendisk->events and ->async_events are added. These should be
initialized by block driver before passing the device to add_disk().
The former contains the mask of all supported events and the latter
the mask of all events which the device can report without polling.
/sys/block/*/events[_async] export these to userland.
* Kernel parameter block.events_dfl_poll_msecs controls the system
polling interval (default is 0 which means disable) and
/sys/block/*/events_poll_msecs control polling intervals for
individual devices (default is -1 meaning use system setting). Note
that if a device can report all supported events asynchronously and
its polling interval isn't explicitly set, the device won't be
polled regardless of the system polling interval.
* If a device is opened exclusively with write access, event checking
is automatically disabled until all write exclusive accesses are
released.
* There are event 'clearing' events. For example, both of currently
defined events are cleared after the device has been successfully
opened. This information is passed to ->check_events() callback
using @clearing argument as a hint.
* Event checking is always performed from system_nrt_wq and timer
slack is set to 25% for polling.
* Nothing changes for drivers which implement ->media_changed() but
not ->check_events(). Going forward, all drivers will be converted
to ->check_events() and ->media_change() will be dropped.
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Kay Sievers <kay.sievers@vrfy.org>
Cc: Jan Kara <jack@suse.cz>
---
block/genhd.c | 429 ++++++++++++++++++++++++++++++++++++++++++++++++
fs/block_dev.c | 41 ++++-
include/linux/blkdev.h | 3 +
include/linux/fs.h | 1 +
include/linux/genhd.h | 18 ++-
5 files changed, 484 insertions(+), 8 deletions(-)
diff --git a/block/genhd.c b/block/genhd.c
index 2e5e4c0..5465a82 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -18,6 +18,7 @@
#include <linux/buffer_head.h>
#include <linux/mutex.h>
#include <linux/idr.h>
+#include <linux/log2.h>
#include "blk.h"
@@ -35,6 +36,10 @@ static DEFINE_IDR(ext_devt_idr);
static struct device_type disk_type;
+static void disk_add_events(struct gendisk *disk);
+static void disk_del_events(struct gendisk *disk);
+static void disk_release_events(struct gendisk *disk);
+
/**
* disk_get_part - get partition
* @disk: disk to look partition from
@@ -609,6 +614,8 @@ void add_disk(struct gendisk *disk)
retval = sysfs_create_link(&disk_to_dev(disk)->kobj, &bdi->dev->kobj,
"bdi");
WARN_ON(retval);
+
+ disk_add_events(disk);
}
EXPORT_SYMBOL(add_disk);
@@ -617,6 +624,8 @@ void del_gendisk(struct gendisk *disk)
struct disk_part_iter piter;
struct hd_struct *part;
+ disk_del_events(disk);
+
/* invalidate stuff */
disk_part_iter_init(&piter, disk,
DISK_PITER_INCL_EMPTY | DISK_PITER_REVERSE);
@@ -1089,6 +1098,7 @@ static void disk_release(struct device *dev)
{
struct gendisk *disk = dev_to_disk(dev);
+ disk_release_events(disk);
kfree(disk->random);
disk_replace_part_tbl(disk, NULL);
free_part_stats(&disk->part0);
@@ -1350,3 +1360,422 @@ int invalidate_partition(struct gendisk *disk, int partno)
}
EXPORT_SYMBOL(invalidate_partition);
+
+/*
+ * Disk events - monitor disk events like media change and eject request.
+ */
+struct disk_events {
+ struct list_head node; /* all disk_event's */
+ struct gendisk *disk; /* the associated disk */
+ spinlock_t lock;
+
+ int block; /* event blocking depth */
+ unsigned int pending; /* events already sent out */
+ unsigned int clearing; /* events being cleared */
+
+ long poll_msecs; /* interval, -1 for default */
+ struct delayed_work dwork;
+};
+
+static const char *disk_events_strs[] = {
+ [ilog2(DISK_EVENT_MEDIA_CHANGE)] = "media_change",
+ [ilog2(DISK_EVENT_EJECT_REQUEST)] = "eject_request",
+};
+
+static char *disk_uevents[] = {
+ [ilog2(DISK_EVENT_MEDIA_CHANGE)] = "DISK_MEDIA_CHANGE=1",
+ [ilog2(DISK_EVENT_EJECT_REQUEST)] = "DISK_EJECT_REQUEST=1",
+};
+
+/* list of all disk_events */
+static DEFINE_MUTEX(disk_events_mutex);
+static LIST_HEAD(disk_events);
+
+/* disable in-kernel polling by default */
+static unsigned long disk_events_dfl_poll_msecs = 0;
+
+static unsigned long disk_events_poll_jiffies(struct gendisk *disk)
+{
+ struct disk_events *ev = disk->ev;
+ long intv_msecs = 0;
+
+ /*
+ * If device-specific poll interval is set, always use it. If
+ * the default is being used, poll iff there are events which
+ * can't be monitored asynchronously.
+ */
+ if (ev->poll_msecs >= 0)
+ intv_msecs = ev->poll_msecs;
+ else if (disk->events & ~disk->async_events)
+ intv_msecs = disk_events_dfl_poll_msecs;
+
+ return msecs_to_jiffies(intv_msecs);
+}
+
+static void __disk_block_events(struct gendisk *disk, bool sync)
+{
+ struct disk_events *ev = disk->ev;
+ unsigned long flags;
+ bool cancel;
+
+ spin_lock_irqsave(&ev->lock, flags);
+ cancel = !ev->block++;
+ spin_unlock_irqrestore(&ev->lock, flags);
+
+ if (cancel) {
+ if (sync)
+ cancel_delayed_work_sync(&disk->ev->dwork);
+ else
+ cancel_delayed_work(&disk->ev->dwork);
+ }
+}
+
+static void __disk_unblock_events(struct gendisk *disk, bool check_now)
+{
+ struct disk_events *ev = disk->ev;
+ unsigned long intv;
+ unsigned long flags;
+
+ spin_lock_irqsave(&ev->lock, flags);
+
+ if (WARN_ON_ONCE(ev->block <= 0))
+ goto out_unlock;
+
+ if (--ev->block)
+ goto out_unlock;
+
+ /*
+ * Not exactly a latency critical operation, set poll timer
+ * slack to 25% and kick event check.
+ */
+ intv = disk_events_poll_jiffies(disk);
+ set_timer_slack(&ev->dwork.timer, intv / 4);
+ if (check_now)
+ queue_delayed_work(system_nrt_wq, &ev->dwork, 0);
+ else if (intv)
+ queue_delayed_work(system_nrt_wq, &ev->dwork, intv);
+out_unlock:
+ spin_unlock_irqrestore(&ev->lock, flags);
+}
+
+/**
+ * disk_block_events - block and flush disk event checking
+ * @disk: disk to block events for
+ *
+ * On return from this function, it is guaranteed that event checking
+ * isn't in progress and won't happen until unblocked by
+ * disk_unblock_events(). Events blocking is counted and the actual
+ * unblocking happens after the matching number of unblocks are done.
+ *
+ * Note that this intentionally does not block event checking from
+ * disk_clear_events().
+ *
+ * CONTEXT:
+ * Might sleep.
+ */
+void disk_block_events(struct gendisk *disk)
+{
+ if (disk->ev)
+ __disk_block_events(disk, true);
+}
+
+/**
+ * disk_unblock_events - unblock disk event checking
+ * @disk: disk to unblock events for
+ *
+ * Undo disk_block_events(). When the block count reaches zero, it
+ * starts events polling if configured.
+ *
+ * CONTEXT:
+ * Don't care. Safe to call from irq context.
+ */
+void disk_unblock_events(struct gendisk *disk)
+{
+ if (disk->ev)
+ __disk_unblock_events(disk, true);
+}
+
+/**
+ * disk_check_events - schedule immediate event checking
+ * @disk: disk to check events for
+ *
+ * Schedule immediate event checking on @disk if not blocked.
+ *
+ * CONTEXT:
+ * Don't care. Safe to call from irq context.
+ */
+void disk_check_events(struct gendisk *disk)
+{
+ if (disk->ev) {
+ __disk_block_events(disk, false);
+ __disk_unblock_events(disk, true);
+ }
+}
+EXPORT_SYMBOL_GPL(disk_check_events);
+
+/**
+ * disk_clear_events - synchronously check, clear and return pending events
+ * @disk: disk to fetch and clear events from
+ * @mask: mask of events to be fetched and clearted
+ *
+ * Disk events are synchronously checked and pending events in @mask
+ * are cleared and returned. This ignores the block count.
+ *
+ * CONTEXT:
+ * Might sleep.
+ */
+unsigned int disk_clear_events(struct gendisk *disk, unsigned int mask)
+{
+ const struct block_device_operations *bdops = disk->fops;
+ struct disk_events *ev = disk->ev;
+ unsigned int pending;
+
+ if (!ev) {
+ /* for drivers still using the old ->media_changed method */
+ if ((mask & DISK_EVENT_MEDIA_CHANGE) &&
+ bdops->media_changed && bdops->media_changed(disk))
+ return DISK_EVENT_MEDIA_CHANGE;
+ return 0;
+ }
+
+ /* tell the workfn about the events being cleared */
+ spin_lock_irq(&ev->lock);
+ ev->clearing |= mask;
+ spin_unlock_irq(&ev->lock);
+
+ /* uncondtionally schedule event check and wait for it to finish */
+ __disk_block_events(disk, true);
+ queue_delayed_work(system_nrt_wq, &ev->dwork, 0);
+ flush_delayed_work(&ev->dwork);
+ __disk_unblock_events(disk, false);
+
+ /* then, fetch and clear pending events */
+ spin_lock_irq(&ev->lock);
+ WARN_ON_ONCE(ev->clearing & mask); /* cleared by workfn */
+ pending = ev->pending & mask;
+ ev->pending &= ~mask;
+ spin_unlock_irq(&ev->lock);
+
+ return pending;
+}
+
+static void disk_events_workfn(struct work_struct *work)
+{
+ struct delayed_work *dwork = to_delayed_work(work);
+ struct disk_events *ev = container_of(dwork, struct disk_events, dwork);
+ struct gendisk *disk = ev->disk;
+ char *envp[ARRAY_SIZE(disk_uevents) + 1] = { };
+ unsigned int clearing = ev->clearing;
+ unsigned int events;
+ unsigned long intv;
+ int nr_events = 0, i;
+
+ /* check events */
+ events = disk->fops->check_events(disk, clearing);
+
+ /* accumulate pending events and schedule next poll if necessary */
+ spin_lock_irq(&ev->lock);
+
+ events &= ~ev->pending;
+ ev->pending |= events;
+ ev->clearing &= ~clearing;
+
+ intv = disk_events_poll_jiffies(disk);
+ if (!ev->block && intv)
+ queue_delayed_work(system_nrt_wq, &ev->dwork, intv);
+
+ spin_unlock_irq(&ev->lock);
+
+ /* tell userland about new events */
+ for (i = 0; i < ARRAY_SIZE(disk_uevents); i++)
+ if (events & (1 << i))
+ envp[nr_events++] = disk_uevents[i];
+
+ if (nr_events)
+ kobject_uevent_env(&disk_to_dev(disk)->kobj, KOBJ_CHANGE, envp);
+}
+
+/*
+ * A disk events enabled device has the following sysfs nodes under
+ * its /sys/block/X/ directory.
+ *
+ * events : list of all supported events
+ * events_async : list of events which can be detected w/o polling
+ * events_poll_msecs : polling interval, 0: disable, -1: system default
+ */
+static ssize_t __disk_events_show(unsigned int events, char *buf)
+{
+ const char *delim = "";
+ ssize_t pos = 0;
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(disk_events_strs); i++)
+ if (events & (1 << i)) {
+ pos += sprintf(buf + pos, "%s%s",
+ delim, disk_events_strs[i]);
+ delim = " ";
+ }
+ if (pos)
+ pos += sprintf(buf + pos, "\n");
+ return pos;
+}
+
+static ssize_t disk_events_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct gendisk *disk = dev_to_disk(dev);
+
+ return __disk_events_show(disk->events, buf);
+}
+
+static ssize_t disk_events_async_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct gendisk *disk = dev_to_disk(dev);
+
+ return __disk_events_show(disk->async_events, buf);
+}
+
+static ssize_t disk_events_poll_msecs_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct gendisk *disk = dev_to_disk(dev);
+
+ return sprintf(buf, "%ld\n", disk->ev->poll_msecs);
+}
+
+static ssize_t disk_events_poll_msecs_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct gendisk *disk = dev_to_disk(dev);
+ long intv;
+
+ if (!count || !sscanf(buf, "%ld", &intv))
+ return -EINVAL;
+
+ if (intv < 0 && intv != -1)
+ return -EINVAL;
+
+ __disk_block_events(disk, true);
+ disk->ev->poll_msecs = intv;
+ __disk_unblock_events(disk, true);
+
+ return count;
+}
+
+static const DEVICE_ATTR(events, S_IRUGO, disk_events_show, NULL);
+static const DEVICE_ATTR(events_async, S_IRUGO, disk_events_async_show, NULL);
+static const DEVICE_ATTR(events_poll_msecs, S_IRUGO|S_IWUSR,
+ disk_events_poll_msecs_show,
+ disk_events_poll_msecs_store);
+
+static const struct attribute *disk_events_attrs[] = {
+ &dev_attr_events.attr,
+ &dev_attr_events_async.attr,
+ &dev_attr_events_poll_msecs.attr,
+ NULL,
+};
+
+/*
+ * The default polling interval can be specified by the kernel
+ * parameter block.events_dfl_poll_msecs which defaults to 0
+ * (disable). This can also be modified runtime by writing to
+ * /sys/module/block/events_dfl_poll_msecs.
+ */
+static int disk_events_set_dfl_poll_msecs(const char *val,
+ const struct kernel_param *kp)
+{
+ struct disk_events *ev;
+ int ret;
+
+ ret = param_set_ulong(val, kp);
+ if (ret < 0)
+ return ret;
+
+ mutex_lock(&disk_events_mutex);
+
+ list_for_each_entry(ev, &disk_events, node)
+ disk_check_events(ev->disk);
+
+ mutex_unlock(&disk_events_mutex);
+
+ return 0;
+}
+
+static const struct kernel_param_ops disk_events_dfl_poll_msecs_param_ops = {
+ .set = disk_events_set_dfl_poll_msecs,
+ .get = param_get_ulong,
+};
+
+#undef MODULE_PARAM_PREFIX
+#define MODULE_PARAM_PREFIX "block."
+
+module_param_cb(events_dfl_poll_msecs, &disk_events_dfl_poll_msecs_param_ops,
+ &disk_events_dfl_poll_msecs, 0644);
+
+/*
+ * disk_{add|del|release}_events - initialize and destroy disk_events.
+ */
+static void disk_add_events(struct gendisk *disk)
+{
+ struct disk_events *ev;
+
+ if (!disk->fops->check_events || !(disk->events | disk->async_events))
+ return;
+
+ ev = kzalloc(sizeof(*ev), GFP_KERNEL);
+ if (!ev) {
+ pr_warn("%s: failed to initialize events\n", disk->disk_name);
+ return;
+ }
+
+ if (sysfs_create_files(&disk_to_dev(disk)->kobj,
+ disk_events_attrs) < 0) {
+ pr_warn("%s: failed to create sysfs files for events\n",
+ disk->disk_name);
+ kfree(ev);
+ return;
+ }
+
+ disk->ev = ev;
+
+ INIT_LIST_HEAD(&ev->node);
+ ev->disk = disk;
+ spin_lock_init(&ev->lock);
+ ev->block = 1;
+ ev->poll_msecs = -1;
+ INIT_DELAYED_WORK(&ev->dwork, disk_events_workfn);
+
+ mutex_lock(&disk_events_mutex);
+ list_add_tail(&ev->node, &disk_events);
+ mutex_unlock(&disk_events_mutex);
+
+ /*
+ * Block count is initialized to 1 and the following initial
+ * unblock kicks it into action.
+ */
+ __disk_unblock_events(disk, true);
+}
+
+static void disk_del_events(struct gendisk *disk)
+{
+ if (!disk->ev)
+ return;
+
+ __disk_block_events(disk, true);
+
+ mutex_lock(&disk_events_mutex);
+ list_del_init(&disk->ev->node);
+ mutex_unlock(&disk_events_mutex);
+
+ sysfs_remove_files(&disk_to_dev(disk)->kobj, disk_events_attrs);
+}
+
+static void disk_release_events(struct gendisk *disk)
+{
+ /* the block count should be 1 from disk_del_events() */
+ WARN_ON_ONCE(disk->ev && disk->ev->block != 1);
+ kfree(disk->ev);
+}
diff --git a/fs/block_dev.c b/fs/block_dev.c
index c1c1b8c..6017389 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -948,10 +948,11 @@ int check_disk_change(struct block_device *bdev)
{
struct gendisk *disk = bdev->bd_disk;
const struct block_device_operations *bdops = disk->fops;
+ unsigned int events;
- if (!bdops->media_changed)
- return 0;
- if (!bdops->media_changed(bdev->bd_disk))
+ events = disk_clear_events(disk, DISK_EVENT_MEDIA_CHANGE |
+ DISK_EVENT_EJECT_REQUEST);
+ if (!(events & DISK_EVENT_MEDIA_CHANGE))
return 0;
flush_disk(bdev);
@@ -1158,9 +1159,10 @@ int blkdev_get(struct block_device *bdev, fmode_t mode, void *holder)
if (whole) {
/* finish claiming */
+ mutex_lock(&bdev->bd_mutex);
spin_lock(&bdev_lock);
- if (res == 0) {
+ if (!res) {
BUG_ON(!bd_may_claim(bdev, whole, holder));
/*
* Note that for a whole device bd_holders
@@ -1180,6 +1182,20 @@ int blkdev_get(struct block_device *bdev, fmode_t mode, void *holder)
wake_up_bit(&whole->bd_claiming, 0);
spin_unlock(&bdev_lock);
+
+ /*
+ * Block event polling for write claims. Any write
+ * holder makes the write_holder state stick until all
+ * are released. This is good enough and tracking
+ * individual writeable reference is too fragile given
+ * the way @mode is used in blkdev_get/put().
+ */
+ if (!res && (mode & FMODE_WRITE) && !bdev->bd_write_holder) {
+ bdev->bd_write_holder = true;
+ disk_block_events(bdev->bd_disk);
+ }
+
+ mutex_unlock(&bdev->bd_mutex);
bdput(whole);
}
@@ -1353,12 +1369,23 @@ int blkdev_put(struct block_device *bdev, fmode_t mode)
spin_unlock(&bdev_lock);
- /* if this was the last claim, holder link should go too */
- if (bdev_free)
+ /*
+ * If this was the last claim, remove holder link and
+ * unblock evpoll if it was a write holder.
+ */
+ if (bdev_free) {
bd_unlink_disk_holder(bdev);
+ if (bdev->bd_write_holder) {
+ disk_unblock_events(bdev->bd_disk);
+ bdev->bd_write_holder = false;
+ } else
+ disk_check_events(bdev->bd_disk);
+ }
mutex_unlock(&bdev->bd_mutex);
- }
+ } else
+ disk_check_events(bdev->bd_disk);
+
return __blkdev_put(bdev, mode, 0);
}
EXPORT_SYMBOL(blkdev_put);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 83031bc..05667e6 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1251,6 +1251,9 @@ struct block_device_operations {
int (*compat_ioctl) (struct block_device *, fmode_t, unsigned, unsigned long);
int (*direct_access) (struct block_device *, sector_t,
void **, unsigned long *);
+ unsigned int (*check_events) (struct gendisk *disk,
+ unsigned int clearing);
+ /* ->media_changed() is DEPRECATED, use ->check_events() instead */
int (*media_changed) (struct gendisk *);
void (*unlock_native_capacity) (struct gendisk *);
int (*revalidate_disk) (struct gendisk *);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index f485015..997d22e 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -662,6 +662,7 @@ struct block_device {
void * bd_claiming;
void * bd_holder;
int bd_holders;
+ bool bd_write_holder;
#ifdef CONFIG_SYSFS
struct gendisk * bd_holder_disk; /* for sysfs slave linkng */
#endif
diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index 56e17ed..13893aa 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -127,6 +127,11 @@ struct hd_struct {
#define GENHD_FL_EXT_DEVT 64 /* allow extended devt */
#define GENHD_FL_NATIVE_CAPACITY 128
+enum {
+ DISK_EVENT_MEDIA_CHANGE = 1 << 0, /* media changed */
+ DISK_EVENT_EJECT_REQUEST = 1 << 1, /* eject requested */
+};
+
#define BLK_SCSI_MAX_CMDS (256)
#define BLK_SCSI_CMD_PER_LONG (BLK_SCSI_MAX_CMDS / (sizeof(long) * 8))
@@ -143,6 +148,8 @@ struct disk_part_tbl {
struct hd_struct __rcu *part[];
};
+struct disk_events;
+
struct gendisk {
/* major, first_minor and minors are input parameters only,
* don't use directly. Use disk_devt() and disk_max_parts().
@@ -154,6 +161,10 @@ struct gendisk {
char disk_name[DISK_NAME_LEN]; /* name of major driver */
char *(*devnode)(struct gendisk *gd, mode_t *mode);
+
+ unsigned int events; /* supported events */
+ unsigned int async_events; /* async events, subset of all */
+
/* Array of pointers to partitions indexed by partno.
* Protected with matching bdev lock but stat and other
* non-critical accesses use RCU. Always access through
@@ -171,8 +182,8 @@ struct gendisk {
struct kobject *slave_dir;
struct timer_rand_state *random;
-
atomic_t sync_io; /* RAID */
+ struct disk_events *ev;
#ifdef CONFIG_BLK_DEV_INTEGRITY
struct blk_integrity *integrity;
#endif
@@ -405,6 +416,11 @@ static inline int get_disk_ro(struct gendisk *disk)
return disk->part0.policy;
}
+extern void disk_block_events(struct gendisk *disk);
+extern void disk_unblock_events(struct gendisk *disk);
+extern void disk_check_events(struct gendisk *disk);
+extern unsigned int disk_clear_events(struct gendisk *disk, unsigned int mask);
+
/* drivers/char/random.c */
extern void add_disk_randomness(struct gendisk *disk);
extern void rand_initialize_disk(struct gendisk *disk);
--
1.7.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH 4/8] cdrom: add ->check_events() support
2010-12-08 19:57 [PATCHSET] block/SCSI: implement in-kernel disk event handling, take#2 Tejun Heo
` (2 preceding siblings ...)
2010-12-08 19:57 ` [PATCH 3/8] implement in-kernel gendisk events handling Tejun Heo
@ 2010-12-08 19:57 ` Tejun Heo
2010-12-08 19:57 ` [PATCH 5/8] scsi: fix TUR error handling in sr_media_change() Tejun Heo
` (4 subsequent siblings)
8 siblings, 0 replies; 33+ messages in thread
From: Tejun Heo @ 2010-12-08 19:57 UTC (permalink / raw)
To: jeff, linux-ide, axboe, linux-kernel, linux-scsi, kay.sievers,
jack, James.Bottomley
Cc: Tejun Heo
In principle, cdrom just needs to pass through ->check_events() but
CDROM_MEDIA_CHANGED ioctl makes things a bit more complex. Just as
with ->media_changed() support, cdrom code needs to buffer the events
and serve them to ioctl and vfs as requested.
As the code has to deal with both ->check_events() and
->media_changed(), and vfs and ioctl event buffering, this patch adds
check_events caching on top of the existing cdi->mc_flags buffering.
It may be a good idea to deprecate CDROM_MEDIA_CHANGED ioctl and
remove all this mess.
Signed-off-by: Tejun Heo <tj@kernel.org>
---
drivers/cdrom/cdrom.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++--
include/linux/cdrom.h | 6 +++++
2 files changed, 58 insertions(+), 3 deletions(-)
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index af13c62..1ea8f8d 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -1348,7 +1348,10 @@ static int cdrom_select_disc(struct cdrom_device_info *cdi, int slot)
if (!CDROM_CAN(CDC_SELECT_DISC))
return -EDRIVE_CANT_DO_THIS;
- (void) cdi->ops->media_changed(cdi, slot);
+ if (cdi->ops->check_events)
+ cdi->ops->check_events(cdi, 0, slot);
+ else
+ cdi->ops->media_changed(cdi, slot);
if (slot == CDSL_NONE) {
/* set media changed bits, on both queues */
@@ -1392,6 +1395,41 @@ static int cdrom_select_disc(struct cdrom_device_info *cdi, int slot)
return slot;
}
+/*
+ * As cdrom implements an extra ioctl consumer for media changed
+ * event, it needs to buffer ->check_events() output, such that event
+ * is not lost for both the usual VFS and ioctl paths.
+ * cdi->{vfs|ioctl}_events are used to buffer pending events for each
+ * path.
+ *
+ * XXX: Locking is non-existent. cdi->ops->check_events() can be
+ * called in parallel and buffering fields are accessed without any
+ * exclusion. The original media_changed code had the same problem.
+ * It might be better to simply deprecate CDROM_MEDIA_CHANGED ioctl
+ * and remove this cruft altogether. It doesn't have much usefulness
+ * at this point.
+ */
+static void cdrom_update_events(struct cdrom_device_info *cdi,
+ unsigned int clearing)
+{
+ unsigned int events;
+
+ events = cdi->ops->check_events(cdi, clearing, CDSL_CURRENT);
+ cdi->vfs_events |= events;
+ cdi->ioctl_events |= events;
+}
+
+unsigned int cdrom_check_events(struct cdrom_device_info *cdi,
+ unsigned int clearing)
+{
+ unsigned int events;
+
+ cdrom_update_events(cdi, clearing);
+ events = cdi->vfs_events;
+ cdi->vfs_events = 0;
+ return events;
+}
+
/* We want to make media_changed accessible to the user through an
* ioctl. The main problem now is that we must double-buffer the
* low-level implementation, to assure that the VFS and the user both
@@ -1403,15 +1441,26 @@ int media_changed(struct cdrom_device_info *cdi, int queue)
{
unsigned int mask = (1 << (queue & 1));
int ret = !!(cdi->mc_flags & mask);
+ bool changed;
if (!CDROM_CAN(CDC_MEDIA_CHANGED))
- return ret;
+ return ret;
+
/* changed since last call? */
- if (cdi->ops->media_changed(cdi, CDSL_CURRENT)) {
+ if (cdi->ops->check_events) {
+ BUG_ON(!queue); /* shouldn't be called from VFS path */
+ cdrom_update_events(cdi, DISK_EVENT_MEDIA_CHANGE);
+ changed = cdi->ioctl_events & DISK_EVENT_MEDIA_CHANGE;
+ cdi->ioctl_events = 0;
+ } else
+ changed = cdi->ops->media_changed(cdi, CDSL_CURRENT);
+
+ if (changed) {
cdi->mc_flags = 0x3; /* set bit on both queues */
ret |= 1;
cdi->media_written = 0;
}
+
cdi->mc_flags &= ~mask; /* clear bit */
return ret;
}
diff --git a/include/linux/cdrom.h b/include/linux/cdrom.h
index 78e9047..35eae4b 100644
--- a/include/linux/cdrom.h
+++ b/include/linux/cdrom.h
@@ -946,6 +946,8 @@ struct cdrom_device_info {
/* device-related storage */
unsigned int options : 30; /* options flags */
unsigned mc_flags : 2; /* media change buffer flags */
+ unsigned int vfs_events; /* cached events for vfs path */
+ unsigned int ioctl_events; /* cached events for ioctl path */
int use_count; /* number of times device opened */
char name[20]; /* name of the device type */
/* per-device flags */
@@ -965,6 +967,8 @@ struct cdrom_device_ops {
int (*open) (struct cdrom_device_info *, int);
void (*release) (struct cdrom_device_info *);
int (*drive_status) (struct cdrom_device_info *, int);
+ unsigned int (*check_events) (struct cdrom_device_info *cdi,
+ unsigned int clearing, int slot);
int (*media_changed) (struct cdrom_device_info *, int);
int (*tray_move) (struct cdrom_device_info *, int);
int (*lock_door) (struct cdrom_device_info *, int);
@@ -993,6 +997,8 @@ extern int cdrom_open(struct cdrom_device_info *cdi, struct block_device *bdev,
extern void cdrom_release(struct cdrom_device_info *cdi, fmode_t mode);
extern int cdrom_ioctl(struct cdrom_device_info *cdi, struct block_device *bdev,
fmode_t mode, unsigned int cmd, unsigned long arg);
+extern unsigned int cdrom_check_events(struct cdrom_device_info *cdi,
+ unsigned int clearing);
extern int cdrom_media_changed(struct cdrom_device_info *);
extern int register_cdrom(struct cdrom_device_info *cdi);
--
1.7.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH 5/8] scsi: fix TUR error handling in sr_media_change()
2010-12-08 19:57 [PATCHSET] block/SCSI: implement in-kernel disk event handling, take#2 Tejun Heo
` (3 preceding siblings ...)
2010-12-08 19:57 ` [PATCH 4/8] cdrom: add ->check_events() support Tejun Heo
@ 2010-12-08 19:57 ` Tejun Heo
2010-12-08 20:14 ` Rolf Eike Beer
` (2 more replies)
2010-12-08 19:57 ` [PATCH 6/8] scsi: replace sr_test_unit_ready() with scsi_test_unit_ready() Tejun Heo
` (3 subsequent siblings)
8 siblings, 3 replies; 33+ messages in thread
From: Tejun Heo @ 2010-12-08 19:57 UTC (permalink / raw)
To: jeff, linux-ide, axboe, linux-kernel, linux-scsi, kay.sievers,
jack, James.Bottomley
Cc: Tejun Heo
sr_test_unit_ready() returns 0 iff TUR succeeded - IOW, when media is
present and the device is actually ready, so the return value wouldn't
be zero when TUR ends with sense data. sr_media_change() incorrectly
tests (retval || (scsi_sense_valid(sshdr)...)) when it tries to test
whether TUR failed without sense data or with sense data indicating
media-not-present.
Fix the test using scsi_status_is_good() and update comments.
Signed-off-by: Tejun Heo <tj@kernel.org>
---
drivers/scsi/sr.c | 18 +++++++++++-------
1 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index d7b383c..deb24f0 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -214,13 +214,17 @@ static int sr_media_change(struct cdrom_device_info *cdi, int slot)
sshdr = kzalloc(sizeof(*sshdr), GFP_KERNEL);
retval = sr_test_unit_ready(cd->device, sshdr);
- if (retval || (scsi_sense_valid(sshdr) &&
- /* 0x3a is medium not present */
- sshdr->asc == 0x3a)) {
- /* Media not present or unable to test, unit probably not
- * ready. This usually means there is no disc in the drive.
- * Mark as changed, and we will figure it out later once
- * the drive is available again.
+ /*
+ * Media is considered to be present if TUR succeeds or fails with
+ * sense data indicating something other than media-not-present
+ * (ASC 0x3a).
+ */
+ if (!scsi_status_is_good(retval) &&
+ (!scsi_sense_valid(sshdr) || sshdr->asc == 0x3a)) {
+ /*
+ * Probably not media in the device. Mark as changed, and
+ * we will figure it out later once the drive is available
+ * again.
*/
cd->device->changed = 1;
/* This will force a flush, if called from check_disk_change */
--
1.7.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 5/8] scsi: fix TUR error handling in sr_media_change()
2010-12-08 19:57 ` [PATCH 5/8] scsi: fix TUR error handling in sr_media_change() Tejun Heo
@ 2010-12-08 20:14 ` Rolf Eike Beer
2010-12-09 10:18 ` [PATCH UPDATED " Tejun Heo
2010-12-09 18:20 ` [PATCH " Sergei Shtylyov
2 siblings, 0 replies; 33+ messages in thread
From: Rolf Eike Beer @ 2010-12-08 20:14 UTC (permalink / raw)
To: Tejun Heo
Cc: jeff, linux-ide, axboe, linux-kernel, linux-scsi, kay.sievers,
jack, James.Bottomley
[-- Attachment #1: Type: Text/Plain, Size: 1110 bytes --]
Tejun Heo wrote:
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -214,13 +214,17 @@ static int sr_media_change(struct cdrom_device_info
> *cdi, int slot)
>
> sshdr = kzalloc(sizeof(*sshdr), GFP_KERNEL);
> retval = sr_test_unit_ready(cd->device, sshdr);
> - if (retval || (scsi_sense_valid(sshdr) &&
> - /* 0x3a is medium not present */
> - sshdr->asc == 0x3a)) {
> - /* Media not present or unable to test, unit probably not
> - * ready. This usually means there is no disc in the drive.
> - * Mark as changed, and we will figure it out later once
> - * the drive is available again.
> + /*
> + * Media is considered to be present if TUR succeeds or fails with
> + * sense data indicating something other than media-not-present
> + * (ASC 0x3a).
> + */
> + if (!scsi_status_is_good(retval) &&
> + (!scsi_sense_valid(sshdr) || sshdr->asc == 0x3a)) {
> + /*
> + * Probably not media in the device. Mark as changed, and
> + * we will figure it out later once the drive is available
> + * again.
"Probably no media ..."?
Eike
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread* [PATCH UPDATED 5/8] scsi: fix TUR error handling in sr_media_change()
2010-12-08 19:57 ` [PATCH 5/8] scsi: fix TUR error handling in sr_media_change() Tejun Heo
2010-12-08 20:14 ` Rolf Eike Beer
@ 2010-12-09 10:18 ` Tejun Heo
2010-12-09 18:20 ` [PATCH " Sergei Shtylyov
2 siblings, 0 replies; 33+ messages in thread
From: Tejun Heo @ 2010-12-09 10:18 UTC (permalink / raw)
To: Tejun Heo
Cc: jeff, linux-ide, axboe, linux-kernel, linux-scsi, kay.sievers,
jack, James.Bottomley, Rolf Eike Beer
sr_test_unit_ready() returns 0 iff TUR succeeded - IOW, when media is
present and the device is actually ready, so the return value wouldn't
be zero when TUR ends with sense data. sr_media_change() incorrectly
tests (retval || (scsi_sense_valid(sshdr)...)) when it tries to test
whether TUR failed without sense data or with sense data indicating
media-not-present.
Fix the test using scsi_status_is_good() and update comments.
- Fixed a comment typo spotted by Eike.
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Rolf Eike Beer <eike-kernel@sf-tec.de>
---
Typo fixed. git tree updated accordingly. Thanks.
drivers/scsi/sr.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
Index: work/drivers/scsi/sr.c
===================================================================
--- work.orig/drivers/scsi/sr.c
+++ work/drivers/scsi/sr.c
@@ -214,13 +214,17 @@ static int sr_media_change(struct cdrom_
sshdr = kzalloc(sizeof(*sshdr), GFP_KERNEL);
retval = sr_test_unit_ready(cd->device, sshdr);
- if (retval || (scsi_sense_valid(sshdr) &&
- /* 0x3a is medium not present */
- sshdr->asc == 0x3a)) {
- /* Media not present or unable to test, unit probably not
- * ready. This usually means there is no disc in the drive.
- * Mark as changed, and we will figure it out later once
- * the drive is available again.
+ /*
+ * Media is considered to be present if TUR succeeds or fails with
+ * sense data indicating something other than media-not-present
+ * (ASC 0x3a).
+ */
+ if (!scsi_status_is_good(retval) &&
+ (!scsi_sense_valid(sshdr) || sshdr->asc == 0x3a)) {
+ /*
+ * Probably no media in the device. Mark as changed, and
+ * we will figure it out later once the drive is available
+ * again.
*/
cd->device->changed = 1;
/* This will force a flush, if called from check_disk_change */
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 5/8] scsi: fix TUR error handling in sr_media_change()
2010-12-08 19:57 ` [PATCH 5/8] scsi: fix TUR error handling in sr_media_change() Tejun Heo
2010-12-08 20:14 ` Rolf Eike Beer
2010-12-09 10:18 ` [PATCH UPDATED " Tejun Heo
@ 2010-12-09 18:20 ` Sergei Shtylyov
2010-12-09 18:53 ` Tejun Heo
2 siblings, 1 reply; 33+ messages in thread
From: Sergei Shtylyov @ 2010-12-09 18:20 UTC (permalink / raw)
To: Tejun Heo
Cc: jeff, linux-ide, axboe, linux-kernel, linux-scsi, kay.sievers,
jack, James.Bottomley
Hello.
Tejun Heo wrote:
> sr_test_unit_ready() returns 0 iff TUR succeeded - IOW, when media is
> present and the device is actually ready, so the return value wouldn't
> be zero when TUR ends with sense data. sr_media_change() incorrectly
> tests (retval || (scsi_sense_valid(sshdr)...)) when it tries to test
> whether TUR failed without sense data or with sense data indicating
> media-not-present.
> Fix the test using scsi_status_is_good() and update comments.
> Signed-off-by: Tejun Heo <tj@kernel.org>
[...]
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index d7b383c..deb24f0 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -214,13 +214,17 @@ static int sr_media_change(struct cdrom_device_info *cdi, int slot)
>
> sshdr = kzalloc(sizeof(*sshdr), GFP_KERNEL);
> retval = sr_test_unit_ready(cd->device, sshdr);
> - if (retval || (scsi_sense_valid(sshdr) &&
> - /* 0x3a is medium not present */
> - sshdr->asc == 0x3a)) {
> - /* Media not present or unable to test, unit probably not
> - * ready. This usually means there is no disc in the drive.
> - * Mark as changed, and we will figure it out later once
> - * the drive is available again.
> + /*
> + * Media is considered to be present if TUR succeeds or fails with
> + * sense data indicating something other than media-not-present
> + * (ASC 0x3a).
> + */
> + if (!scsi_status_is_good(retval) &&
> + (!scsi_sense_valid(sshdr) || sshdr->asc == 0x3a)) {
> + /*
> + * Probably not media in the device. Mark as changed, and
s/not/no/
WBR, Sergei
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH 5/8] scsi: fix TUR error handling in sr_media_change()
2010-12-09 18:20 ` [PATCH " Sergei Shtylyov
@ 2010-12-09 18:53 ` Tejun Heo
0 siblings, 0 replies; 33+ messages in thread
From: Tejun Heo @ 2010-12-09 18:53 UTC (permalink / raw)
To: Sergei Shtylyov
Cc: jeff, linux-ide, axboe, linux-kernel, linux-scsi, kay.sievers,
jack, James.Bottomley
Hello, Sergei.
On 12/09/2010 07:20 PM, Sergei Shtylyov wrote:
>> + /*
>> + * Media is considered to be present if TUR succeeds or fails with
>> + * sense data indicating something other than media-not-present
>> + * (ASC 0x3a).
>> + */
>> + if (!scsi_status_is_good(retval) &&
>> + (!scsi_sense_valid(sshdr) || sshdr->asc == 0x3a)) {
>> + /*
>> + * Probably not media in the device. Mark as changed, and
>
> s/not/no/
Updated patch already posted. Thanks.
--
tejun
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 6/8] scsi: replace sr_test_unit_ready() with scsi_test_unit_ready()
2010-12-08 19:57 [PATCHSET] block/SCSI: implement in-kernel disk event handling, take#2 Tejun Heo
` (4 preceding siblings ...)
2010-12-08 19:57 ` [PATCH 5/8] scsi: fix TUR error handling in sr_media_change() Tejun Heo
@ 2010-12-08 19:57 ` Tejun Heo
2010-12-08 19:57 ` [PATCH 7/8] sr: implement sr_check_events() Tejun Heo
` (2 subsequent siblings)
8 siblings, 0 replies; 33+ messages in thread
From: Tejun Heo @ 2010-12-08 19:57 UTC (permalink / raw)
To: jeff, linux-ide, axboe, linux-kernel, linux-scsi, kay.sievers,
jack, James.Bottomley
Cc: Tejun Heo
The usage of TUR has been confusing involving several different
commits updating different parts over time. Currently, the only
differences between scsi_test_unit_ready() and sr_test_unit_ready()
are,
* scsi_test_unit_ready() also sets sdev->changed on NOT_READY.
* scsi_test_unit_ready() returns 0 if TUR ended with UNIT_ATTENTION or
NOT_READY.
Due to the above two differences, sr is using its own
sr_test_unit_ready(), but sd - the sole user of the above extra
handling - doesn't even need them.
Where scsi_test_unit_ready() is used in sd_media_changed(), the code
is looking for device ready w/ media present state which is true iff
TUR succeeds w/o sense data or UA, and when the device is not ready
for whatever reason sd_media_changed() explicitly marks media as
missing so there's no reason to set sdev->changed automatically from
scsi_test_unit_ready() on NOT_READY.
Drop both special handlings from scsi_test_unit_ready(), which makes
it equivalant to sr_test_unit_ready(), and replace
sr_test_unit_ready() with scsi_test_unit_ready(). Also, drop the
unnecessary explicit NOT_READY check from sd_media_changed().
Checking return value is enough for testing device readiness.
Signed-off-by: Tejun Heo <tj@kernel.org>
---
drivers/scsi/scsi_lib.c | 13 +------------
drivers/scsi/sd.c | 10 +---------
drivers/scsi/sr.c | 31 +++----------------------------
drivers/scsi/sr.h | 1 -
drivers/scsi/sr_ioctl.c | 2 +-
5 files changed, 6 insertions(+), 51 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index eafeeda..13bf891 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1984,8 +1984,7 @@ EXPORT_SYMBOL(scsi_mode_sense);
* in.
*
* Returns zero if unsuccessful or an error if TUR failed. For
- * removable media, a return of NOT_READY or UNIT_ATTENTION is
- * translated to success, with the ->changed flag updated.
+ * removable media, UNIT_ATTENTION sets ->changed flag.
**/
int
scsi_test_unit_ready(struct scsi_device *sdev, int timeout, int retries,
@@ -2012,16 +2011,6 @@ scsi_test_unit_ready(struct scsi_device *sdev, int timeout, int retries,
} while (scsi_sense_valid(sshdr) &&
sshdr->sense_key == UNIT_ATTENTION && --retries);
- if (!sshdr)
- /* could not allocate sense buffer, so can't process it */
- return result;
-
- if (sdev->removable && scsi_sense_valid(sshdr) &&
- (sshdr->sense_key == UNIT_ATTENTION ||
- sshdr->sense_key == NOT_READY)) {
- sdev->changed = 1;
- result = 0;
- }
if (!sshdr_external)
kfree(sshdr);
return result;
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index b9ab3a5..8d488a9 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1045,15 +1045,7 @@ static int sd_media_changed(struct gendisk *disk)
sshdr);
}
- /*
- * Unable to test, unit probably not ready. This usually
- * means there is no disc in the drive. Mark as changed,
- * and we will figure it out later once the drive is
- * available again.
- */
- if (retval || (scsi_sense_valid(sshdr) &&
- /* 0x3a is medium not present */
- sshdr->asc == 0x3a)) {
+ if (retval) {
set_media_not_present(sdkp);
retval = 1;
goto out;
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index deb24f0..f6d8ee4 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -165,32 +165,6 @@ static void scsi_cd_put(struct scsi_cd *cd)
mutex_unlock(&sr_ref_mutex);
}
-/* identical to scsi_test_unit_ready except that it doesn't
- * eat the NOT_READY returns for removable media */
-int sr_test_unit_ready(struct scsi_device *sdev, struct scsi_sense_hdr *sshdr)
-{
- int retries = MAX_RETRIES;
- int the_result;
- u8 cmd[] = {TEST_UNIT_READY, 0, 0, 0, 0, 0 };
-
- /* issue TEST_UNIT_READY until the initial startup UNIT_ATTENTION
- * conditions are gone, or a timeout happens
- */
- do {
- the_result = scsi_execute_req(sdev, cmd, DMA_NONE, NULL,
- 0, sshdr, SR_TIMEOUT,
- retries--, NULL);
- if (scsi_sense_valid(sshdr) &&
- sshdr->sense_key == UNIT_ATTENTION)
- sdev->changed = 1;
-
- } while (retries > 0 &&
- (!scsi_status_is_good(the_result) ||
- (scsi_sense_valid(sshdr) &&
- sshdr->sense_key == UNIT_ATTENTION)));
- return the_result;
-}
-
/*
* This function checks to see if the media has been changed in the
* CDROM drive. It is possible that we have already sensed a change,
@@ -213,7 +187,8 @@ static int sr_media_change(struct cdrom_device_info *cdi, int slot)
}
sshdr = kzalloc(sizeof(*sshdr), GFP_KERNEL);
- retval = sr_test_unit_ready(cd->device, sshdr);
+ retval = scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES,
+ sshdr);
/*
* Media is considered to be present if TUR succeeds or fails with
* sense data indicating something other than media-not-present
@@ -784,7 +759,7 @@ static void get_capabilities(struct scsi_cd *cd)
}
/* eat unit attentions */
- sr_test_unit_ready(cd->device, &sshdr);
+ scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
/* ask for mode page 0x2a */
rc = scsi_mode_sense(cd->device, 0, 0x2a, buffer, 128,
diff --git a/drivers/scsi/sr.h b/drivers/scsi/sr.h
index 1e144df..81fbc0b 100644
--- a/drivers/scsi/sr.h
+++ b/drivers/scsi/sr.h
@@ -61,7 +61,6 @@ int sr_select_speed(struct cdrom_device_info *cdi, int speed);
int sr_audio_ioctl(struct cdrom_device_info *, unsigned int, void *);
int sr_is_xa(Scsi_CD *);
-int sr_test_unit_ready(struct scsi_device *sdev, struct scsi_sense_hdr *sshdr);
/* sr_vendor.c */
void sr_vendor_init(Scsi_CD *);
diff --git a/drivers/scsi/sr_ioctl.c b/drivers/scsi/sr_ioctl.c
index 3cd8ffb..8be3055 100644
--- a/drivers/scsi/sr_ioctl.c
+++ b/drivers/scsi/sr_ioctl.c
@@ -307,7 +307,7 @@ int sr_drive_status(struct cdrom_device_info *cdi, int slot)
/* we have no changer support */
return -EINVAL;
}
- if (0 == sr_test_unit_ready(cd->device, &sshdr))
+ if (!scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr))
return CDS_DISC_OK;
/* SK/ASC/ASCQ of 2/4/1 means "unit is becoming ready" */
--
1.7.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH 7/8] sr: implement sr_check_events()
2010-12-08 19:57 [PATCHSET] block/SCSI: implement in-kernel disk event handling, take#2 Tejun Heo
` (5 preceding siblings ...)
2010-12-08 19:57 ` [PATCH 6/8] scsi: replace sr_test_unit_ready() with scsi_test_unit_ready() Tejun Heo
@ 2010-12-08 19:57 ` Tejun Heo
2011-01-30 1:26 ` Simon Arlott
2010-12-08 19:57 ` [PATCH 8/8] sd: implement sd_check_events() Tejun Heo
2010-12-16 16:31 ` [PATCHSET] block/SCSI: implement in-kernel disk event handling, take#2 Tejun Heo
8 siblings, 1 reply; 33+ messages in thread
From: Tejun Heo @ 2010-12-08 19:57 UTC (permalink / raw)
To: jeff, linux-ide, axboe, linux-kernel, linux-scsi, kay.sievers,
jack, James.Bottomley
Cc: Tejun Heo
Replace sr_media_change() with sr_check_events(). It normally only
uses GET_EVENT_STATUS_NOTIFICATION to check both media change and
eject request. If @clearing includes DISK_EVENT_MEDIA_CHANGE, it
issues TUR and compares whether media presence has changed. The SCSI
specific media change uevent is kept for compatibility.
sr_media_change() was doing both media change check and revalidation.
The revalidation part is split into sr_block_revalidate_disk().
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Kay Sievers <kay.sievers@vrfy.org>
---
drivers/scsi/sr.c | 147 +++++++++++++++++++++++++++++++++------------------
drivers/scsi/sr.h | 2 +-
include/scsi/scsi.h | 1 +
3 files changed, 98 insertions(+), 52 deletions(-)
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index f6d8ee4..1e4b9b1 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -104,14 +104,15 @@ static void sr_release(struct cdrom_device_info *);
static void get_sectorsize(struct scsi_cd *);
static void get_capabilities(struct scsi_cd *);
-static int sr_media_change(struct cdrom_device_info *, int);
+static unsigned int sr_check_events(struct cdrom_device_info *cdi,
+ unsigned int clearing, int slot);
static int sr_packet(struct cdrom_device_info *, struct packet_command *);
static struct cdrom_device_ops sr_dops = {
.open = sr_open,
.release = sr_release,
.drive_status = sr_drive_status,
- .media_changed = sr_media_change,
+ .check_events = sr_check_events,
.tray_move = sr_tray_move,
.lock_door = sr_lock_door,
.select_speed = sr_select_speed,
@@ -165,69 +166,96 @@ static void scsi_cd_put(struct scsi_cd *cd)
mutex_unlock(&sr_ref_mutex);
}
+static unsigned int sr_get_events(struct scsi_device *sdev)
+{
+ u8 buf[8];
+ u8 cmd[] = { GET_EVENT_STATUS_NOTIFICATION,
+ 1, /* polled */
+ 0, 0, /* reserved */
+ 1 << 4, /* notification class: media */
+ 0, 0, /* reserved */
+ 0, sizeof(buf), /* allocation length */
+ 0, /* control */
+ };
+ struct event_header *eh = (void *)buf;
+ struct media_event_desc *med = (void *)(buf + 4);
+ struct scsi_sense_hdr sshdr;
+ int result;
+
+ result = scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf, sizeof(buf),
+ &sshdr, SR_TIMEOUT, MAX_RETRIES, NULL);
+ if (scsi_sense_valid(&sshdr) && sshdr.sense_key == UNIT_ATTENTION)
+ return DISK_EVENT_MEDIA_CHANGE;
+
+ if (result || be16_to_cpu(eh->data_len) < sizeof(*med))
+ return 0;
+
+ if (eh->nea || eh->notification_class != 0x4)
+ return 0;
+
+ if (med->media_event_code == 1)
+ return DISK_EVENT_EJECT_REQUEST;
+ else if (med->media_event_code == 2)
+ return DISK_EVENT_MEDIA_CHANGE;
+ return 0;
+}
+
/*
- * This function checks to see if the media has been changed in the
- * CDROM drive. It is possible that we have already sensed a change,
- * or the drive may have sensed one and not yet reported it. We must
- * be ready for either case. This function always reports the current
- * value of the changed bit. If flag is 0, then the changed bit is reset.
- * This function could be done as an ioctl, but we would need to have
- * an inode for that to work, and we do not always have one.
+ * This function checks to see if the media has been changed or eject
+ * button has been pressed. It is possible that we have already
+ * sensed a change, or the drive may have sensed one and not yet
+ * reported it. The past events are accumulated in sdev->changed and
+ * returned together with the current state.
*/
-
-static int sr_media_change(struct cdrom_device_info *cdi, int slot)
+static unsigned int sr_check_events(struct cdrom_device_info *cdi,
+ unsigned int clearing, int slot)
{
struct scsi_cd *cd = cdi->handle;
- int retval;
- struct scsi_sense_hdr *sshdr;
+ bool last_present;
+ struct scsi_sense_hdr sshdr;
+ unsigned int events;
+ int ret;
- if (CDSL_CURRENT != slot) {
- /* no changer support */
- return -EINVAL;
- }
+ /* no changer support */
+ if (CDSL_CURRENT != slot)
+ return 0;
- sshdr = kzalloc(sizeof(*sshdr), GFP_KERNEL);
- retval = scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES,
- sshdr);
+ events = sr_get_events(cd->device);
+ /*
+ * GET_EVENT_STATUS_NOTIFICATION is enough unless MEDIA_CHANGE
+ * is being cleared. Note that there are devices which hang
+ * if asked to execute TUR repeatedly.
+ */
+ if (!(clearing & DISK_EVENT_MEDIA_CHANGE))
+ goto skip_tur;
+
+ /* let's see whether the media is there with TUR */
+ last_present = cd->media_present;
+ ret = scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
/*
* Media is considered to be present if TUR succeeds or fails with
* sense data indicating something other than media-not-present
* (ASC 0x3a).
*/
- if (!scsi_status_is_good(retval) &&
- (!scsi_sense_valid(sshdr) || sshdr->asc == 0x3a)) {
- /*
- * Probably not media in the device. Mark as changed, and
- * we will figure it out later once the drive is available
- * again.
- */
- cd->device->changed = 1;
- /* This will force a flush, if called from check_disk_change */
- retval = 1;
- goto out;
- };
+ cd->media_present = scsi_status_is_good(ret) ||
+ (scsi_sense_valid(&sshdr) && sshdr.asc != 0x3a);
- retval = cd->device->changed;
- cd->device->changed = 0;
- /* If the disk changed, the capacity will now be different,
- * so we force a re-read of this information */
- if (retval) {
- /* check multisession offset etc */
- sr_cd_check(cdi);
- get_sectorsize(cd);
+ if (last_present != cd->media_present)
+ events |= DISK_EVENT_MEDIA_CHANGE;
+skip_tur:
+ if (cd->device->changed) {
+ events |= DISK_EVENT_MEDIA_CHANGE;
+ cd->device->changed = 0;
}
-out:
- /* Notify userspace, that media has changed. */
- if (retval != cd->previous_state)
+ /* for backward compatibility */
+ if (events & DISK_EVENT_MEDIA_CHANGE)
sdev_evt_send_simple(cd->device, SDEV_EVT_MEDIA_CHANGE,
GFP_KERNEL);
- cd->previous_state = retval;
- kfree(sshdr);
- return retval;
+ return events;
}
-
+
/*
* sr_done is the interrupt routine for the device driver.
*
@@ -512,10 +540,25 @@ out:
return ret;
}
-static int sr_block_media_changed(struct gendisk *disk)
+static unsigned int sr_block_check_events(struct gendisk *disk,
+ unsigned int clearing)
{
struct scsi_cd *cd = scsi_cd(disk);
- return cdrom_media_changed(&cd->cdi);
+ return cdrom_check_events(&cd->cdi, clearing);
+}
+
+static int sr_block_revalidate_disk(struct gendisk *disk)
+{
+ struct scsi_cd *cd = scsi_cd(disk);
+ struct scsi_sense_hdr sshdr;
+
+ /* if the unit is not ready, nothing more to do */
+ if (scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr))
+ return 0;
+
+ sr_cd_check(&cd->cdi);
+ get_sectorsize(cd);
+ return 0;
}
static const struct block_device_operations sr_bdops =
@@ -524,7 +567,8 @@ static const struct block_device_operations sr_bdops =
.open = sr_block_open,
.release = sr_block_release,
.ioctl = sr_block_ioctl,
- .media_changed = sr_block_media_changed,
+ .check_events = sr_block_check_events,
+ .revalidate_disk = sr_block_revalidate_disk,
/*
* No compat_ioctl for now because sr_block_ioctl never
* seems to pass arbitary ioctls down to host drivers.
@@ -597,6 +641,7 @@ static int sr_probe(struct device *dev)
sprintf(disk->disk_name, "sr%d", minor);
disk->fops = &sr_bdops;
disk->flags = GENHD_FL_CD;
+ disk->events = DISK_EVENT_MEDIA_CHANGE | DISK_EVENT_EJECT_REQUEST;
blk_queue_rq_timeout(sdev->request_queue, SR_TIMEOUT);
@@ -606,7 +651,7 @@ static int sr_probe(struct device *dev)
cd->disk = disk;
cd->capacity = 0x1fffff;
cd->device->changed = 1; /* force recheck CD type */
- cd->previous_state = 1;
+ cd->media_present = 1;
cd->use = 1;
cd->readcd_known = 0;
cd->readcd_cdda = 0;
diff --git a/drivers/scsi/sr.h b/drivers/scsi/sr.h
index 81fbc0b..e036f1d 100644
--- a/drivers/scsi/sr.h
+++ b/drivers/scsi/sr.h
@@ -40,7 +40,7 @@ typedef struct scsi_cd {
unsigned xa_flag:1; /* CD has XA sectors ? */
unsigned readcd_known:1; /* drive supports READ_CD (0xbe) */
unsigned readcd_cdda:1; /* reading audio data using READ_CD */
- unsigned previous_state:1; /* media has changed */
+ unsigned media_present:1; /* media is present */
struct cdrom_device_info cdi;
/* We hold gendisk and scsi_device references on probe and use
* the refs on this kref to decide when to release them */
diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h
index 216af85..8675861 100644
--- a/include/scsi/scsi.h
+++ b/include/scsi/scsi.h
@@ -104,6 +104,7 @@ struct scsi_cmnd;
#define UNMAP 0x42
#define READ_TOC 0x43
#define READ_HEADER 0x44
+#define GET_EVENT_STATUS_NOTIFICATION 0x4a
#define LOG_SELECT 0x4c
#define LOG_SENSE 0x4d
#define XDWRITEREAD_10 0x53
--
1.7.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 7/8] sr: implement sr_check_events()
2010-12-08 19:57 ` [PATCH 7/8] sr: implement sr_check_events() Tejun Heo
@ 2011-01-30 1:26 ` Simon Arlott
2011-01-30 1:31 ` [PATCH] cdrom: support devices that have check_events but not media_changed Simon Arlott
2011-02-10 17:30 ` [PATCH 7/8] sr: implement sr_check_events() ael
0 siblings, 2 replies; 33+ messages in thread
From: Simon Arlott @ 2011-01-30 1:26 UTC (permalink / raw)
To: Tejun Heo
Cc: jeff, linux-ide, axboe, linux-kernel, linux-scsi, kay.sievers,
jack, James.Bottomley
On 08/12/10 19:57, Tejun Heo wrote:
> Replace sr_media_change() with sr_check_events(). It normally only
> uses GET_EVENT_STATUS_NOTIFICATION to check both media change and
> eject request. If @clearing includes DISK_EVENT_MEDIA_CHANGE, it
> issues TUR and compares whether media presence has changed. The SCSI
> specific media change uevent is kept for compatibility.
>
> sr_media_change() was doing both media change check and revalidation.
> The revalidation part is split into sr_block_revalidate_disk().
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Kay Sievers <kay.sievers@vrfy.org>
> ---
> drivers/scsi/sr.c | 147 +++++++++++++++++++++++++++++++++------------------
> drivers/scsi/sr.h | 2 +-
> include/scsi/scsi.h | 1 +
> 3 files changed, 98 insertions(+), 52 deletions(-)
This breaks growisofs:
:-( /dev/dvd: CD_ROM_MEDIA_CHANGED ioctl failed: Inappropriate ioctl for device
SR_CAPABILITIES in sr.c includes CDC_MEDIA_CHANGED but cdrom.c removes
this capability in register_cdrom because there is no media_changed op:
ENSURE(media_changed, CDC_MEDIA_CHANGED);
The ioctl function cdrom_ioctl_media_changed() then returns -ENOSYS.
The media_changed() function again checks this capability but never uses
the media_changed op if the check_events op exists.
The cdrom_media_changed() function requires the media_changed op but
then calls media_changed().
It looks like cdrom_select_disc() is going to dereference the NULL
media_changed pointer if both check_events and media_changed don't
exist.
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index f6d8ee4..1e4b9b1 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -104,14 +104,15 @@ static void sr_release(struct cdrom_device_info *);
> static void get_sectorsize(struct scsi_cd *);
> static void get_capabilities(struct scsi_cd *);
>
> -static int sr_media_change(struct cdrom_device_info *, int);
> +static unsigned int sr_check_events(struct cdrom_device_info *cdi,
> + unsigned int clearing, int slot);
> static int sr_packet(struct cdrom_device_info *, struct packet_command *);
>
> static struct cdrom_device_ops sr_dops = {
> .open = sr_open,
> .release = sr_release,
> .drive_status = sr_drive_status,
> - .media_changed = sr_media_change,
> + .check_events = sr_check_events,
> .tray_move = sr_tray_move,
> .lock_door = sr_lock_door,
> .select_speed = sr_select_speed,
> @@ -165,69 +166,96 @@ static void scsi_cd_put(struct scsi_cd *cd)
> mutex_unlock(&sr_ref_mutex);
> }
--
Simon Arlott
^ permalink raw reply [flat|nested] 33+ messages in thread* [PATCH] cdrom: support devices that have check_events but not media_changed
2011-01-30 1:26 ` Simon Arlott
@ 2011-01-30 1:31 ` Simon Arlott
2011-01-31 10:12 ` Tejun Heo
2011-01-31 11:22 ` [PATCH] " Sergei Shtylyov
2011-02-10 17:30 ` [PATCH 7/8] sr: implement sr_check_events() ael
1 sibling, 2 replies; 33+ messages in thread
From: Simon Arlott @ 2011-01-30 1:31 UTC (permalink / raw)
To: axboe
Cc: Tejun Heo, jeff, linux-ide, linux-kernel, linux-scsi,
kay.sievers, jack, James.Bottomley
93aae17af1172c40c6f74b7294e93a90c3cfaa5d replaced the media_changed op
with the check_events op in drivers/scsi/sr.c
All users that check for the CDC_MEDIA_CHANGED capability try both
the check_events op and the media_changed op, but register_cdrom()
was requiring media_changed.
This patch fixes the capability checking and removes a redundant
check that media_changed != NULL.
The cdrom_select_disc ioctl is also using the two operations, so
they should be required for CDC_SELECT_DISC too.
Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
Cc: Jens Axboe <axboe@kernel.dk>
Cc: Tejun Heo <tj@kernel.org>
Cc: Kay Sievers <kay.sievers@vrfy.org>
---
drivers/cdrom/cdrom.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 14033a3..4716d76 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -409,7 +409,9 @@ int register_cdrom(struct cdrom_device_info *cdi)
}
ENSURE(drive_status, CDC_DRIVE_STATUS );
- ENSURE(media_changed, CDC_MEDIA_CHANGED);
+ if (cdo->check_events == NULL && cdo->media_changed == NULL) {
+ *change_capability = ~(CDC_MEDIA_CHANGED | CDC_SELECT_DISC);
+ }
ENSURE(tray_move, CDC_CLOSE_TRAY | CDC_OPEN_TRAY);
ENSURE(lock_door, CDC_LOCK);
ENSURE(select_speed, CDC_SELECT_SPEED);
@@ -1471,7 +1473,7 @@ int cdrom_media_changed(struct cdrom_device_info *cdi)
/* This talks to the VFS, which doesn't like errors - just 1 or 0.
* Returning "0" is always safe (media hasn't been changed). Do that
* if the low-level cdrom driver dosn't support media changed. */
- if (cdi == NULL || cdi->ops->media_changed == NULL)
+ if (cdi == NULL)
return 0;
if (!CDROM_CAN(CDC_MEDIA_CHANGED))
return 0;
--
1.7.0.4
--
Simon Arlott
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH] cdrom: support devices that have check_events but not media_changed
2011-01-30 1:31 ` [PATCH] cdrom: support devices that have check_events but not media_changed Simon Arlott
@ 2011-01-31 10:12 ` Tejun Heo
2011-01-31 18:26 ` [PATCH (v2)] " Simon Arlott
2011-01-31 11:22 ` [PATCH] " Sergei Shtylyov
1 sibling, 1 reply; 33+ messages in thread
From: Tejun Heo @ 2011-01-31 10:12 UTC (permalink / raw)
To: Simon Arlott
Cc: axboe, jeff, linux-ide, linux-kernel, linux-scsi, kay.sievers,
jack, James.Bottomley
Hello,
On Sun, Jan 30, 2011 at 01:31:16AM +0000, Simon Arlott wrote:
> 93aae17af1172c40c6f74b7294e93a90c3cfaa5d replaced the media_changed op
> with the check_events op in drivers/scsi/sr.c
>
> All users that check for the CDC_MEDIA_CHANGED capability try both
> the check_events op and the media_changed op, but register_cdrom()
> was requiring media_changed.
>
> This patch fixes the capability checking and removes a redundant
> check that media_changed != NULL.
>
> The cdrom_select_disc ioctl is also using the two operations, so
> they should be required for CDC_SELECT_DISC too.
First of all, thanks a lot for fixing this. Just minor nits below.
> diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
> index 14033a3..4716d76 100644
> --- a/drivers/cdrom/cdrom.c
> +++ b/drivers/cdrom/cdrom.c
> @@ -409,7 +409,9 @@ int register_cdrom(struct cdrom_device_info *cdi)
> }
>
> ENSURE(drive_status, CDC_DRIVE_STATUS );
> - ENSURE(media_changed, CDC_MEDIA_CHANGED);
> + if (cdo->check_events == NULL && cdo->media_changed == NULL) {
> + *change_capability = ~(CDC_MEDIA_CHANGED | CDC_SELECT_DISC);
> + }
Can we lose the unnecessary {}?
> ENSURE(tray_move, CDC_CLOSE_TRAY | CDC_OPEN_TRAY);
> ENSURE(lock_door, CDC_LOCK);
> ENSURE(select_speed, CDC_SELECT_SPEED);
> @@ -1471,7 +1473,7 @@ int cdrom_media_changed(struct cdrom_device_info *cdi)
> /* This talks to the VFS, which doesn't like errors - just 1 or 0.
> * Returning "0" is always safe (media hasn't been changed). Do that
> * if the low-level cdrom driver dosn't support media changed. */
> - if (cdi == NULL || cdi->ops->media_changed == NULL)
> + if (cdi == NULL)
This change is probably not necessary as cdrom_media_changed() is only
called from drivers which implement media_changed. I don't think it's
gonna break anything but the whole media_changed stuff is scheduled
for removal anyway, so I think it would be better to keep the changes
minimal.
Thank you.
--
tejun
^ permalink raw reply [flat|nested] 33+ messages in thread* [PATCH (v2)] cdrom: support devices that have check_events but not media_changed
2011-01-31 10:12 ` Tejun Heo
@ 2011-01-31 18:26 ` Simon Arlott
0 siblings, 0 replies; 33+ messages in thread
From: Simon Arlott @ 2011-01-31 18:26 UTC (permalink / raw)
To: Tejun Heo
Cc: axboe, jeff, linux-ide, linux-kernel, linux-scsi, kay.sievers,
jack, James.Bottomley, sshtylyov
Commit 93aae17af1172c40c6f74b7294e93a90c3cfaa5d ("sr: implement
sr_check_events()") replaced the media_changed op with the
check_events op in drivers/scsi/sr.c
All users that check for the CDC_MEDIA_CHANGED capbility try both
the check_events op and the media_changed op, but register_cdrom()
was requiring media_changed.
This patch fixes the capability checking.
The cdrom_select_disc ioctl is also using the two operations, so
they should be required for CDC_SELECT_DISC too.
Signed-off-by: Simon Arlott <simon@fire.lp0.eu>
Cc: Tejun Heo <tj@kernel.org>
Cc: Kay Sievers <kay.sievers@vrfy.org>
---
drivers/cdrom/cdrom.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index 14033a3..e2c48a7 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -409,7 +409,8 @@ int register_cdrom(struct cdrom_device_info *cdi)
}
ENSURE(drive_status, CDC_DRIVE_STATUS );
- ENSURE(media_changed, CDC_MEDIA_CHANGED);
+ if (cdo->check_events == NULL && cdo->media_changed == NULL)
+ *change_capability = ~(CDC_MEDIA_CHANGED | CDC_SELECT_DISC);
ENSURE(tray_move, CDC_CLOSE_TRAY | CDC_OPEN_TRAY);
ENSURE(lock_door, CDC_LOCK);
ENSURE(select_speed, CDC_SELECT_SPEED);
--
1.7.0.4
--
Simon Arlott
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH] cdrom: support devices that have check_events but not media_changed
2011-01-30 1:31 ` [PATCH] cdrom: support devices that have check_events but not media_changed Simon Arlott
2011-01-31 10:12 ` Tejun Heo
@ 2011-01-31 11:22 ` Sergei Shtylyov
1 sibling, 0 replies; 33+ messages in thread
From: Sergei Shtylyov @ 2011-01-31 11:22 UTC (permalink / raw)
To: Simon Arlott
Cc: axboe, Tejun Heo, jeff, linux-ide, linux-kernel, linux-scsi,
kay.sievers, jack, James.Bottomley
Hello.
On 30-01-2011 4:31, Simon Arlott wrote:
> 93aae17af1172c40c6f74b7294e93a90c3cfaa5d replaced the media_changed op
Please specify the commit summary in parens, as asked by Linus.
> with the check_events op in drivers/scsi/sr.c
> All users that check for the CDC_MEDIA_CHANGED capability try both
> the check_events op and the media_changed op, but register_cdrom()
> was requiring media_changed.
> This patch fixes the capability checking and removes a redundant
> check that media_changed != NULL.
> The cdrom_select_disc ioctl is also using the two operations, so
> they should be required for CDC_SELECT_DISC too.
> Signed-off-by: Simon Arlott<simon@fire.lp0.eu>
> Cc: Jens Axboe<axboe@kernel.dk>
> Cc: Tejun Heo<tj@kernel.org>
> Cc: Kay Sievers<kay.sievers@vrfy.org>
[...]
> diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
> index 14033a3..4716d76 100644
> --- a/drivers/cdrom/cdrom.c
> +++ b/drivers/cdrom/cdrom.c
> @@ -409,7 +409,9 @@ int register_cdrom(struct cdrom_device_info *cdi)
> }
>
> ENSURE(drive_status, CDC_DRIVE_STATUS );
> - ENSURE(media_changed, CDC_MEDIA_CHANGED);
> + if (cdo->check_events == NULL && cdo->media_changed == NULL) {
> + *change_capability = ~(CDC_MEDIA_CHANGED | CDC_SELECT_DISC);
> + }
{} are not needed here. I think checkstatus.pl should detect this...
WBR, Sergei
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 7/8] sr: implement sr_check_events()
2011-01-30 1:26 ` Simon Arlott
2011-01-30 1:31 ` [PATCH] cdrom: support devices that have check_events but not media_changed Simon Arlott
@ 2011-02-10 17:30 ` ael
1 sibling, 0 replies; 33+ messages in thread
From: ael @ 2011-02-10 17:30 UTC (permalink / raw)
To: linux-kernel
Not sure whether these patches are responsible, but I have been seeing
problems mounting dvd media since 2.6.38-rc3. It improved slightly with
2.6.38-rc4, but I have to use 2.6.38-rc1 (rc2 not tested) to mount some
of my dvd-rw media.
Message is (from memory & approximately) media already mounted or device
busy after
mount /dev/dvd /dvd
Apologies for such a vague report not properly investigated, but maybe.
or maybe not, better than nothing.
ael
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 8/8] sd: implement sd_check_events()
2010-12-08 19:57 [PATCHSET] block/SCSI: implement in-kernel disk event handling, take#2 Tejun Heo
` (6 preceding siblings ...)
2010-12-08 19:57 ` [PATCH 7/8] sr: implement sr_check_events() Tejun Heo
@ 2010-12-08 19:57 ` Tejun Heo
2010-12-16 16:31 ` [PATCHSET] block/SCSI: implement in-kernel disk event handling, take#2 Tejun Heo
8 siblings, 0 replies; 33+ messages in thread
From: Tejun Heo @ 2010-12-08 19:57 UTC (permalink / raw)
To: jeff, linux-ide, axboe, linux-kernel, linux-scsi, kay.sievers,
jack, James.Bottomley
Cc: Tejun Heo
Replace sd_media_change() with sd_check_events(). sd used to set the
changed state whenever the device is not ready, which can cause event
loop while the device is not ready. Media presence handling code is
changed such that the changed state is set iff the media presence
actually changes. UA still always sets the changed state and
NOT_READY always (at least where it used to set ->changed) clears
media presence, so no event is lost.
Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Kay Sievers <kay.sievers@vrfy.org>
---
drivers/scsi/sd.c | 87 ++++++++++++++++++++++++++++------------------------
drivers/scsi/sd.h | 1 -
2 files changed, 47 insertions(+), 41 deletions(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 8d488a9..b179b0e 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -991,30 +991,50 @@ out:
static void set_media_not_present(struct scsi_disk *sdkp)
{
+ if (sdkp->media_present)
+ sdkp->device->changed = 1;
sdkp->media_present = 0;
sdkp->capacity = 0;
- sdkp->device->changed = 1;
+}
+
+static int media_not_present(struct scsi_disk *sdkp,
+ struct scsi_sense_hdr *sshdr)
+{
+ if (!scsi_sense_valid(sshdr))
+ return 0;
+
+ /* not invoked for commands that could return deferred errors */
+ switch (sshdr->sense_key) {
+ case UNIT_ATTENTION:
+ sdkp->device->changed = 1;
+ /* fall through */
+ case NOT_READY:
+ /* medium not present */
+ if (sshdr->asc == 0x3A) {
+ set_media_not_present(sdkp);
+ return 1;
+ }
+ }
+ return 0;
}
/**
- * sd_media_changed - check if our medium changed
- * @disk: kernel device descriptor
+ * sd_check_events - check media events
+ * @disk: kernel device descriptor
+ * @clearing: disk events currently being cleared
*
- * Returns 0 if not applicable or no change; 1 if change
+ * Returns mask of DISK_EVENT_*.
*
* Note: this function is invoked from the block subsystem.
**/
-static int sd_media_changed(struct gendisk *disk)
+static unsigned int sd_check_events(struct gendisk *disk, unsigned int clearing)
{
struct scsi_disk *sdkp = scsi_disk(disk);
struct scsi_device *sdp = sdkp->device;
struct scsi_sense_hdr *sshdr = NULL;
int retval;
- SCSI_LOG_HLQUEUE(3, sd_printk(KERN_INFO, sdkp, "sd_media_changed\n"));
-
- if (!sdp->removable)
- return 0;
+ SCSI_LOG_HLQUEUE(3, sd_printk(KERN_INFO, sdkp, "sd_check_events\n"));
/*
* If the device is offline, don't send any commands - just pretend as
@@ -1024,7 +1044,6 @@ static int sd_media_changed(struct gendisk *disk)
*/
if (!scsi_device_online(sdp)) {
set_media_not_present(sdkp);
- retval = 1;
goto out;
}
@@ -1045,26 +1064,30 @@ static int sd_media_changed(struct gendisk *disk)
sshdr);
}
- if (retval) {
+ /* failed to execute TUR, assume media not present */
+ if (host_byte(retval)) {
set_media_not_present(sdkp);
- retval = 1;
goto out;
}
+ if (media_not_present(sdkp, sshdr))
+ goto out;
+
/*
* For removable scsi disk we have to recognise the presence
- * of a disk in the drive. This is kept in the struct scsi_disk
- * struct and tested at open ! Daniel Roche (dan@lectra.fr)
+ * of a disk in the drive.
*/
+ if (!sdkp->media_present)
+ sdp->changed = 1;
sdkp->media_present = 1;
-
- retval = sdp->changed;
- sdp->changed = 0;
out:
- if (retval != sdkp->previous_state)
+ /* for backward compatibility */
+ if (sdp->changed)
sdev_evt_send_simple(sdp, SDEV_EVT_MEDIA_CHANGE, GFP_KERNEL);
- sdkp->previous_state = retval;
kfree(sshdr);
+
+ retval = sdp->changed ? DISK_EVENT_MEDIA_CHANGE : 0;
+ sdp->changed = 0;
return retval;
}
@@ -1157,7 +1180,7 @@ static const struct block_device_operations sd_fops = {
#ifdef CONFIG_COMPAT
.compat_ioctl = sd_compat_ioctl,
#endif
- .media_changed = sd_media_changed,
+ .check_events = sd_check_events,
.revalidate_disk = sd_revalidate_disk,
.unlock_native_capacity = sd_unlock_native_capacity,
};
@@ -1293,23 +1316,6 @@ static int sd_done(struct scsi_cmnd *SCpnt)
return good_bytes;
}
-static int media_not_present(struct scsi_disk *sdkp,
- struct scsi_sense_hdr *sshdr)
-{
-
- if (!scsi_sense_valid(sshdr))
- return 0;
- /* not invoked for commands that could return deferred errors */
- if (sshdr->sense_key != NOT_READY &&
- sshdr->sense_key != UNIT_ATTENTION)
- return 0;
- if (sshdr->asc != 0x3A) /* medium not present */
- return 0;
-
- set_media_not_present(sdkp);
- return 1;
-}
-
/*
* spinup disk - called only in sd_revalidate_disk()
*/
@@ -1484,7 +1490,7 @@ static void read_capacity_error(struct scsi_disk *sdkp, struct scsi_device *sdp,
*/
if (sdp->removable &&
sense_valid && sshdr->sense_key == NOT_READY)
- sdp->changed = 1;
+ set_media_not_present(sdkp);
/*
* We used to set media_present to 0 here to indicate no media
@@ -2339,8 +2345,10 @@ static void sd_probe_async(void *data, async_cookie_t cookie)
gd->driverfs_dev = &sdp->sdev_gendev;
gd->flags = GENHD_FL_EXT_DEVT;
- if (sdp->removable)
+ if (sdp->removable) {
gd->flags |= GENHD_FL_REMOVABLE;
+ gd->events |= DISK_EVENT_MEDIA_CHANGE;
+ }
add_disk(gd);
sd_dif_config_host(sdkp);
@@ -2422,7 +2430,6 @@ static int sd_probe(struct device *dev)
sdkp->disk = gd;
sdkp->index = index;
atomic_set(&sdkp->openers, 0);
- sdkp->previous_state = 1;
if (!sdp->request_queue->rq_timeout) {
if (sdp->type != TYPE_MOD)
diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index 55488fa..c9d8f6c 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -55,7 +55,6 @@ struct scsi_disk {
u8 media_present;
u8 write_prot;
u8 protection_type;/* Data Integrity Field */
- unsigned previous_state : 1;
unsigned ATO : 1; /* state of disk ATO bit */
unsigned WCE : 1; /* state of disk WCE bit */
unsigned RCD : 1; /* state of disk RCD bit, unused */
--
1.7.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCHSET] block/SCSI: implement in-kernel disk event handling, take#2
2010-12-08 19:57 [PATCHSET] block/SCSI: implement in-kernel disk event handling, take#2 Tejun Heo
` (7 preceding siblings ...)
2010-12-08 19:57 ` [PATCH 8/8] sd: implement sd_check_events() Tejun Heo
@ 2010-12-16 16:31 ` Tejun Heo
2010-12-16 16:36 ` Jens Axboe
2010-12-16 17:00 ` Christoph Hellwig
8 siblings, 2 replies; 33+ messages in thread
From: Tejun Heo @ 2010-12-16 16:31 UTC (permalink / raw)
To: jeff, linux-ide, axboe, linux-kernel, linux-scsi, kay.sievers,
jack, James.Bottomley
On 12/08/2010 08:57 PM, Tejun Heo wrote:
> This is the second take of in-kernel-disk-event patchset which
> implements in-kernel disk event handling framework and adds support
> for it to sr and sd. This is largely to move media presence polling
> into kernel as userspace implementation turned out to be quite
> problematic over the years.
Kay, Jens, James, how does this look to you guys?
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCHSET] block/SCSI: implement in-kernel disk event handling, take#2
2010-12-16 16:31 ` [PATCHSET] block/SCSI: implement in-kernel disk event handling, take#2 Tejun Heo
@ 2010-12-16 16:36 ` Jens Axboe
2010-12-16 16:38 ` James Bottomley
2010-12-16 16:41 ` Kay Sievers
2010-12-16 17:00 ` Christoph Hellwig
1 sibling, 2 replies; 33+ messages in thread
From: Jens Axboe @ 2010-12-16 16:36 UTC (permalink / raw)
To: Tejun Heo
Cc: jeff, linux-ide, linux-kernel, linux-scsi, kay.sievers, jack,
James.Bottomley
On 2010-12-16 17:31, Tejun Heo wrote:
> On 12/08/2010 08:57 PM, Tejun Heo wrote:
>> This is the second take of in-kernel-disk-event patchset which
>> implements in-kernel disk event handling framework and adds support
>> for it to sr and sd. This is largely to move media presence polling
>> into kernel as userspace implementation turned out to be quite
>> problematic over the years.
>
> Kay, Jens, James, how does this look to you guys?
I like the concept, this is probably what we should have done all along.
The user space method has been tried and failed. So I was mostly laying
it low waiting for feedback before integrating this.
Kay?
--
Jens Axboe
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHSET] block/SCSI: implement in-kernel disk event handling, take#2
2010-12-16 16:36 ` Jens Axboe
@ 2010-12-16 16:38 ` James Bottomley
2010-12-16 16:44 ` Kay Sievers
2010-12-16 16:41 ` Kay Sievers
1 sibling, 1 reply; 33+ messages in thread
From: James Bottomley @ 2010-12-16 16:38 UTC (permalink / raw)
To: Jens Axboe
Cc: Tejun Heo, jeff, linux-ide, linux-kernel, linux-scsi, kay.sievers, jack
On Thu, 2010-12-16 at 17:36 +0100, Jens Axboe wrote:
> On 2010-12-16 17:31, Tejun Heo wrote:
> > On 12/08/2010 08:57 PM, Tejun Heo wrote:
> >> This is the second take of in-kernel-disk-event patchset which
> >> implements in-kernel disk event handling framework and adds support
> >> for it to sr and sd. This is largely to move media presence polling
> >> into kernel as userspace implementation turned out to be quite
> >> problematic over the years.
> >
> > Kay, Jens, James, how does this look to you guys?
>
> I like the concept, this is probably what we should have done all along.
> The user space method has been tried and failed. So I was mostly laying
> it low waiting for feedback before integrating this.
Me too pretty much ... event driven is nice, but it only really works if
the userspace stuff will stop polling ...
James
> Kay?
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHSET] block/SCSI: implement in-kernel disk event handling, take#2
2010-12-16 16:38 ` James Bottomley
@ 2010-12-16 16:44 ` Kay Sievers
0 siblings, 0 replies; 33+ messages in thread
From: Kay Sievers @ 2010-12-16 16:44 UTC (permalink / raw)
To: James Bottomley
Cc: Jens Axboe, Tejun Heo, jeff, linux-ide, linux-kernel, linux-scsi, jack
On Thu, Dec 16, 2010 at 17:38, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Thu, 2010-12-16 at 17:36 +0100, Jens Axboe wrote:
>> On 2010-12-16 17:31, Tejun Heo wrote:
>> > On 12/08/2010 08:57 PM, Tejun Heo wrote:
>> >> This is the second take of in-kernel-disk-event patchset which
>> >> implements in-kernel disk event handling framework and adds support
>> >> for it to sr and sd. This is largely to move media presence polling
>> >> into kernel as userspace implementation turned out to be quite
>> >> problematic over the years.
>> >
>> > Kay, Jens, James, how does this look to you guys?
>>
>> I like the concept, this is probably what we should have done all along.
>> The user space method has been tried and failed. So I was mostly laying
>> it low waiting for feedback before integrating this.
>
> Me too pretty much ... event driven is nice, but it only really works if
> the userspace stuff will stop polling ...
David Zeuthen and I will take care of it, and leave the devices alone
when the kernel tells us it's not needed to poll from userspace.
Kay
--
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
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHSET] block/SCSI: implement in-kernel disk event handling, take#2
@ 2010-12-16 16:44 ` Kay Sievers
0 siblings, 0 replies; 33+ messages in thread
From: Kay Sievers @ 2010-12-16 16:44 UTC (permalink / raw)
To: James Bottomley
Cc: Jens Axboe, Tejun Heo, jeff, linux-ide, linux-kernel, linux-scsi, jack
On Thu, Dec 16, 2010 at 17:38, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Thu, 2010-12-16 at 17:36 +0100, Jens Axboe wrote:
>> On 2010-12-16 17:31, Tejun Heo wrote:
>> > On 12/08/2010 08:57 PM, Tejun Heo wrote:
>> >> This is the second take of in-kernel-disk-event patchset which
>> >> implements in-kernel disk event handling framework and adds support
>> >> for it to sr and sd. This is largely to move media presence polling
>> >> into kernel as userspace implementation turned out to be quite
>> >> problematic over the years.
>> >
>> > Kay, Jens, James, how does this look to you guys?
>>
>> I like the concept, this is probably what we should have done all along.
>> The user space method has been tried and failed. So I was mostly laying
>> it low waiting for feedback before integrating this.
>
> Me too pretty much ... event driven is nice, but it only really works if
> the userspace stuff will stop polling ...
David Zeuthen and I will take care of it, and leave the devices alone
when the kernel tells us it's not needed to poll from userspace.
Kay
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHSET] block/SCSI: implement in-kernel disk event handling, take#2
2010-12-16 16:36 ` Jens Axboe
2010-12-16 16:38 ` James Bottomley
@ 2010-12-16 16:41 ` Kay Sievers
2010-12-16 16:43 ` Jens Axboe
1 sibling, 1 reply; 33+ messages in thread
From: Kay Sievers @ 2010-12-16 16:41 UTC (permalink / raw)
To: Jens Axboe
Cc: Tejun Heo, jeff, linux-ide, linux-kernel, linux-scsi, jack,
James.Bottomley
On Thu, Dec 16, 2010 at 17:36, Jens Axboe <axboe@kernel.dk> wrote:
> On 2010-12-16 17:31, Tejun Heo wrote:
>> On 12/08/2010 08:57 PM, Tejun Heo wrote:
>>> This is the second take of in-kernel-disk-event patchset which
>>> implements in-kernel disk event handling framework and adds support
>>> for it to sr and sd. This is largely to move media presence polling
>>> into kernel as userspace implementation turned out to be quite
>>> problematic over the years.
>>
>> Kay, Jens, James, how does this look to you guys?
>
> I like the concept, this is probably what we should have done all along.
> The user space method has been tried and failed. So I was mostly laying
> it low waiting for feedback before integrating this.
Yeah, it's nice stuff. Userspace works, but can not be made entirely
safe. We have (rare) bugs we just can not fix otherwise than disabling
polling.
> Kay?
I want it! :)
If you merge it, let me know when you have a tree ready, so I can test
the most recent version again.
Thanks,
Kay
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHSET] block/SCSI: implement in-kernel disk event handling, take#2
2010-12-16 16:41 ` Kay Sievers
@ 2010-12-16 16:43 ` Jens Axboe
2010-12-16 16:45 ` Tejun Heo
2010-12-16 16:55 ` Jens Axboe
0 siblings, 2 replies; 33+ messages in thread
From: Jens Axboe @ 2010-12-16 16:43 UTC (permalink / raw)
To: Kay Sievers
Cc: Tejun Heo, jeff, linux-ide, linux-kernel, linux-scsi, jack,
James.Bottomley
On 2010-12-16 17:41, Kay Sievers wrote:
> On Thu, Dec 16, 2010 at 17:36, Jens Axboe <axboe@kernel.dk> wrote:
>> On 2010-12-16 17:31, Tejun Heo wrote:
>>> On 12/08/2010 08:57 PM, Tejun Heo wrote:
>>>> This is the second take of in-kernel-disk-event patchset which
>>>> implements in-kernel disk event handling framework and adds support
>>>> for it to sr and sd. This is largely to move media presence polling
>>>> into kernel as userspace implementation turned out to be quite
>>>> problematic over the years.
>>>
>>> Kay, Jens, James, how does this look to you guys?
>>
>> I like the concept, this is probably what we should have done all along.
>> The user space method has been tried and failed. So I was mostly laying
>> it low waiting for feedback before integrating this.
>
> Yeah, it's nice stuff. Userspace works, but can not be made entirely
> safe. We have (rare) bugs we just can not fix otherwise than disabling
> polling.
>
>> Kay?
>
> I want it! :)
>
> If you merge it, let me know when you have a tree ready, so I can test
> the most recent version again.
OK good, looks like we all agree... I'll queue it up in for-2.6.38
branch and update for-next to have all that stuff.
--
Jens Axboe
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHSET] block/SCSI: implement in-kernel disk event handling, take#2
2010-12-16 16:43 ` Jens Axboe
@ 2010-12-16 16:45 ` Tejun Heo
2010-12-16 17:00 ` Jens Axboe
2010-12-16 16:55 ` Jens Axboe
1 sibling, 1 reply; 33+ messages in thread
From: Tejun Heo @ 2010-12-16 16:45 UTC (permalink / raw)
To: Jens Axboe
Cc: Kay Sievers, jeff, linux-ide, linux-kernel, linux-scsi, jack,
James.Bottomley
Hello,
On 12/16/2010 05:43 PM, Jens Axboe wrote:
>>>> Kay, Jens, James, how does this look to you guys?
>>>
>>> I like the concept, this is probably what we should have done all along.
>>> The user space method has been tried and failed. So I was mostly laying
>>> it low waiting for feedback before integrating this.
>>
>> Yeah, it's nice stuff. Userspace works, but can not be made entirely
>> safe. We have (rare) bugs we just can not fix otherwise than disabling
>> polling.
>>
>>> Kay?
>>
>> I want it! :)
>>
>> If you merge it, let me know when you have a tree ready, so I can test
>> the most recent version again.
>
> OK good, looks like we all agree... I'll queue it up in for-2.6.38
> branch and update for-next to have all that stuff.
Heh, that was fast. Anyways, James, it would be great if you can take
a look at the sr/sd changes. I think they're correct but that's kinda
expected, so... :-)
--
tejun
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHSET] block/SCSI: implement in-kernel disk event handling, take#2
2010-12-16 16:45 ` Tejun Heo
@ 2010-12-16 17:00 ` Jens Axboe
2010-12-16 18:11 ` Tejun Heo
0 siblings, 1 reply; 33+ messages in thread
From: Jens Axboe @ 2010-12-16 17:00 UTC (permalink / raw)
To: Tejun Heo
Cc: Kay Sievers, jeff, linux-ide, linux-kernel, linux-scsi, jack,
James.Bottomley
On 2010-12-16 17:45, Tejun Heo wrote:
> Hello,
>
> On 12/16/2010 05:43 PM, Jens Axboe wrote:
>>>>> Kay, Jens, James, how does this look to you guys?
>>>>
>>>> I like the concept, this is probably what we should have done all along.
>>>> The user space method has been tried and failed. So I was mostly laying
>>>> it low waiting for feedback before integrating this.
>>>
>>> Yeah, it's nice stuff. Userspace works, but can not be made entirely
>>> safe. We have (rare) bugs we just can not fix otherwise than disabling
>>> polling.
>>>
>>>> Kay?
>>>
>>> I want it! :)
>>>
>>> If you merge it, let me know when you have a tree ready, so I can test
>>> the most recent version again.
>>
>> OK good, looks like we all agree... I'll queue it up in for-2.6.38
>> branch and update for-next to have all that stuff.
>
> Heh, that was fast. Anyways, James, it would be great if you can take
> a look at the sr/sd changes. I think they're correct but that's kinda
> expected, so... :-)
One thing I noticed is that cdrom_check_events() needs an export.
--
Jens Axboe
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHSET] block/SCSI: implement in-kernel disk event handling, take#2
2010-12-16 17:00 ` Jens Axboe
@ 2010-12-16 18:11 ` Tejun Heo
0 siblings, 0 replies; 33+ messages in thread
From: Tejun Heo @ 2010-12-16 18:11 UTC (permalink / raw)
To: Jens Axboe
Cc: Kay Sievers, jeff, linux-ide, linux-kernel, linux-scsi, jack,
James.Bottomley
On 12/16/2010 06:00 PM, Jens Axboe wrote:
>> Heh, that was fast. Anyways, James, it would be great if you can take
>> a look at the sr/sd changes. I think they're correct but that's kinda
>> expected, so... :-)
>
> One thing I noticed is that cdrom_check_events() needs an export.
Yeap, indeed. Thanks for taking care of it.
--
tejun
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHSET] block/SCSI: implement in-kernel disk event handling, take#2
2010-12-16 16:43 ` Jens Axboe
@ 2010-12-16 16:55 ` Jens Axboe
2010-12-16 16:55 ` Jens Axboe
1 sibling, 0 replies; 33+ messages in thread
From: Jens Axboe @ 2010-12-16 16:55 UTC (permalink / raw)
To: Kay Sievers
Cc: Tejun Heo, jeff, linux-ide, linux-kernel, linux-scsi, jack,
James.Bottomley
On 2010-12-16 17:43, Jens Axboe wrote:
> On 2010-12-16 17:41, Kay Sievers wrote:
>> On Thu, Dec 16, 2010 at 17:36, Jens Axboe <axboe@kernel.dk> wrote:
>>> On 2010-12-16 17:31, Tejun Heo wrote:
>>>> On 12/08/2010 08:57 PM, Tejun Heo wrote:
>>>>> This is the second take of in-kernel-disk-event patchset which
>>>>> implements in-kernel disk event handling framework and adds support
>>>>> for it to sr and sd. This is largely to move media presence polling
>>>>> into kernel as userspace implementation turned out to be quite
>>>>> problematic over the years.
>>>>
>>>> Kay, Jens, James, how does this look to you guys?
>>>
>>> I like the concept, this is probably what we should have done all along.
>>> The user space method has been tried and failed. So I was mostly laying
>>> it low waiting for feedback before integrating this.
>>
>> Yeah, it's nice stuff. Userspace works, but can not be made entirely
>> safe. We have (rare) bugs we just can not fix otherwise than disabling
>> polling.
>>
>>> Kay?
>>
>> I want it! :)
>>
>> If you merge it, let me know when you have a tree ready, so I can test
>> the most recent version again.
>
> OK good, looks like we all agree... I'll queue it up in for-2.6.38
> branch and update for-next to have all that stuff.
Done. It's in for-2.6.38/event-handling, and for-next is update to have
the pending stuff for 2.6.38 in total.
--
Jens Axboe
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHSET] block/SCSI: implement in-kernel disk event handling, take#2
@ 2010-12-16 16:55 ` Jens Axboe
0 siblings, 0 replies; 33+ messages in thread
From: Jens Axboe @ 2010-12-16 16:55 UTC (permalink / raw)
To: Kay Sievers
Cc: Tejun Heo, jeff, linux-ide, linux-kernel, linux-scsi, jack,
James.Bottomley
On 2010-12-16 17:43, Jens Axboe wrote:
> On 2010-12-16 17:41, Kay Sievers wrote:
>> On Thu, Dec 16, 2010 at 17:36, Jens Axboe <axboe@kernel.dk> wrote:
>>> On 2010-12-16 17:31, Tejun Heo wrote:
>>>> On 12/08/2010 08:57 PM, Tejun Heo wrote:
>>>>> This is the second take of in-kernel-disk-event patchset which
>>>>> implements in-kernel disk event handling framework and adds support
>>>>> for it to sr and sd. This is largely to move media presence polling
>>>>> into kernel as userspace implementation turned out to be quite
>>>>> problematic over the years.
>>>>
>>>> Kay, Jens, James, how does this look to you guys?
>>>
>>> I like the concept, this is probably what we should have done all along.
>>> The user space method has been tried and failed. So I was mostly laying
>>> it low waiting for feedback before integrating this.
>>
>> Yeah, it's nice stuff. Userspace works, but can not be made entirely
>> safe. We have (rare) bugs we just can not fix otherwise than disabling
>> polling.
>>
>>> Kay?
>>
>> I want it! :)
>>
>> If you merge it, let me know when you have a tree ready, so I can test
>> the most recent version again.
>
> OK good, looks like we all agree... I'll queue it up in for-2.6.38
> branch and update for-next to have all that stuff.
Done. It's in for-2.6.38/event-handling, and for-next is update to have
the pending stuff for 2.6.38 in total.
--
Jens Axboe
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHSET] block/SCSI: implement in-kernel disk event handling, take#2
2010-12-16 16:31 ` [PATCHSET] block/SCSI: implement in-kernel disk event handling, take#2 Tejun Heo
2010-12-16 16:36 ` Jens Axboe
@ 2010-12-16 17:00 ` Christoph Hellwig
2010-12-16 18:04 ` Tejun Heo
1 sibling, 1 reply; 33+ messages in thread
From: Christoph Hellwig @ 2010-12-16 17:00 UTC (permalink / raw)
To: Tejun Heo
Cc: jeff, linux-ide, axboe, linux-kernel, linux-scsi, kay.sievers,
jack, James.Bottomley
On Thu, Dec 16, 2010 at 05:31:29PM +0100, Tejun Heo wrote:
> On 12/08/2010 08:57 PM, Tejun Heo wrote:
> > This is the second take of in-kernel-disk-event patchset which
> > implements in-kernel disk event handling framework and adds support
> > for it to sr and sd. This is largely to move media presence polling
> > into kernel as userspace implementation turned out to be quite
> > problematic over the years.
>
> Kay, Jens, James, how does this look to you guys?
Any chance could could conver all block drivers to the new methods
instead of leaving us with yet another incomplete transition?
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCHSET] block/SCSI: implement in-kernel disk event handling, take#2
2010-12-16 17:00 ` Christoph Hellwig
@ 2010-12-16 18:04 ` Tejun Heo
0 siblings, 0 replies; 33+ messages in thread
From: Tejun Heo @ 2010-12-16 18:04 UTC (permalink / raw)
To: Christoph Hellwig
Cc: jeff, linux-ide, axboe, linux-kernel, linux-scsi, kay.sievers,
jack, James.Bottomley
Hey, Christoph.
On 12/16/2010 06:00 PM, Christoph Hellwig wrote:
> Any chance could could conver all block drivers to the new methods
> instead of leaving us with yet another incomplete transition?
Yeah, sure. I wanted to make sure the approach is agreed on before
going forward with the rest of changes. It's not like we have a lot
of .media_changed in the kernel anyway. The only problem is that some
drivers are quite obscure at this point and would be pretty difficult
to verify, but then again if nobody can noticie the breakage, maybe
it's not broken at all. Once things settle down, I'll go ahead with
the rest of the conversion.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 33+ messages in thread