Device sysfs locking reconsidered to remove recently introduced deadlock possibility.

git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@1361 d57e44dd-8a1f-0410-8b47-8ef2f437770f
This commit is contained in:
Vladislav Bolkhovitin
2009-11-26 18:39:13 +00:00
parent a4f3af18bb
commit f381dbb74c
5 changed files with 167 additions and 172 deletions

View File

@@ -1811,6 +1811,9 @@ struct scst_device {
*/
struct rw_semaphore dev_attr_rwsem;
/* Used to serialize all the device sysfs calls */
struct mutex dev_sysfs_mutex;
struct kobject dev_kobj; /* kobject for this struct */
struct kobject *dev_exp_kobj; /* exported groups */

View File

@@ -277,12 +277,6 @@ struct vdev_type_funcs {
void (*vdev_del) (struct scst_vdisk_dev *virt_dev);
struct scst_vdisk_dev *(*vdev_find) (const char *name);
int (*parse_cmd) (struct vdev_type *vdt, char *p, int *action);
/* Supposed to be called under scst_vdisk_mutex */
int (*perform_cmd) (struct vdev_type *vdt, int action, char *p,
char *name);
int (*parse_option) (struct scst_vdisk_dev *virt_dev, char *p);
int (*pre_register) (struct scst_vdisk_dev *virt_dev);
@@ -404,6 +398,9 @@ static ssize_t vdisk_sysfs_filename_show(struct kobject *kobj,
static ssize_t vdisk_sysfs_resync_size_store(struct kobject *kobj,
struct kobj_attribute *attr, const char *buf, size_t count);
static ssize_t vcdrom_sysfs_filename_store(struct kobject *kobj,
struct kobj_attribute *attr, const char *buf, size_t count);
static struct kobj_attribute vdisk_size_attr =
__ATTR(size, S_IRUGO, vdisk_sysfs_size_show, NULL);
static struct kobj_attribute vdisk_blocksize_attr =
@@ -423,6 +420,10 @@ static struct kobj_attribute vdisk_filename_attr =
static struct kobj_attribute vdisk_resync_size_attr =
__ATTR(resync_size, S_IWUSR, NULL, vdisk_sysfs_resync_size_store);
static struct kobj_attribute vcdrom_filename_attr =
__ATTR(filename, S_IRUGO|S_IWUSR, vdisk_sysfs_filename_show,
vcdrom_sysfs_filename_store);
static const struct attribute *vdisk_fileio_attrs[] = {
&vdisk_size_attr.attr,
&vdisk_blocksize_attr.attr,
@@ -457,12 +458,13 @@ static const struct attribute *vdisk_nullio_attrs[] = {
static const struct attribute *vcdrom_attrs[] = {
&vdisk_size_attr.attr,
&vdisk_removable_attr.attr,
&vdisk_filename_attr.attr,
&vcdrom_filename_attr.attr,
NULL,
};
#endif /* CONFIG_SCST_PROC */
/* Protects vdisks addition/deletion and related activities, like search */
static DEFINE_MUTEX(scst_vdisk_mutex);
/* Both protected by scst_vdisk_mutex */
@@ -2923,12 +2925,16 @@ static void vdisk_report_registering(const struct scst_vdisk_dev *virt_dev)
return;
}
/* scst_vdisk_mutex supposed to be held */
static int __vdisk_resync_size(struct scst_vdisk_dev *virt_dev)
static int vdisk_resync_size(struct scst_vdisk_dev *virt_dev)
{
loff_t file_size;
int res = 0;
/*
* There's no need in any lock here, because SCST core serializes
* all device sysfs calls.
*/
if (!virt_dev->nullio) {
res = vdisk_get_file_size(virt_dev->file_name,
virt_dev->blockio, &file_size);
@@ -2969,26 +2975,6 @@ out:
return res;
}
/* scst_vdisk_mutex supposed to be held */
static int vdisk_resync_size(struct vdev_type *vdt, char *p, const char *name)
{
struct scst_vdisk_dev *virt_dev;
int res;
virt_dev = vdt->vfns.vdev_find(name);
if (virt_dev == NULL) {
PRINT_ERROR("Device %s not found", name);
res = -EINVAL;
goto out;
}
res = __vdisk_resync_size(virt_dev);
out:
TRACE_EXIT_RES(res);
return res;
}
static void vdev_init(struct vdev_type *vdt, struct scst_vdisk_dev *virt_dev)
{
memset(virt_dev, 0, sizeof(*virt_dev));
@@ -3393,37 +3379,6 @@ static struct scst_vdisk_dev *vcdrom_find(const char *name)
return res;
}
static int vdisk_parse_cmd(struct vdev_type *vdt, char *p, int *action)
{
int res = -EINVAL;
TRACE_ENTRY();
if (!strncmp("resync_size", p, 11)) {
res = 11;
p += res;
*action = VDISK_ACTION_RESYNC_SIZE;
}
TRACE_EXIT_RES(res);
return res;
}
/* scst_vdisk_mutex supposed to be held */
static int vdisk_perform_cmd(struct vdev_type *vdt, int action, char *p,
char *name)
{
int res = -EINVAL;
TRACE_ENTRY();
if (action == VDISK_ACTION_RESYNC_SIZE)
res = vdisk_resync_size(vdt, p, name);
TRACE_EXIT_RES(res);
return res;
}
static void vcdrom_init(struct vdev_type *vdt,
struct scst_vdisk_dev *virt_dev)
{
@@ -3460,29 +3415,48 @@ static int vcdrom_pre_register(struct scst_vdisk_dev *virt_dev)
return res;
}
/* scst_vdisk_mutex supposed to be held */
static int vcdrom_change(struct vdev_type *vdt, char *p, const char *name)
static int vcdrom_change(struct scst_vdisk_dev *virt_dev,
const char *buffer, int length)
{
loff_t err;
struct scst_vdisk_dev *virt_dev;
char *file_name, *fn = NULL, *old_fn;
int len;
char *old_fn, *i_buf, *p, *pp;
const char *file_name = NULL;
int res = 0;
virt_dev = vdt->vfns.vdev_find(name);
if (virt_dev == NULL) {
PRINT_ERROR("Virtual device with name "
"%s not found", name);
res = -EINVAL;
TRACE_ENTRY();
/*
* There's no need in any lock here, because SCST core serializes
* all device sysfs calls.
*/
i_buf = kmalloc(length+1, GFP_KERNEL);
if (i_buf == NULL) {
PRINT_ERROR("Unable to alloc intermediate buffer with size %d",
length+1);
res = -ENOMEM;
goto out;
}
memcpy(i_buf, buffer, length);
i_buf[length] = '\0';
p = i_buf;
while (isspace(*p) && *p != '\0')
p++;
file_name = p;
while (!isspace(*p) && *p != '\0')
p++;
*p++ = '\0';
p = &i_buf[length-1];
pp = p;
while (isspace(*p) && *p != '\0') {
pp = p;
p--;
}
*pp = '\0';
res = scst_suspend_activity(true);
if (res != 0)
goto out;
if (*file_name == '\0') {
virt_dev->cdrom_empty = 1;
TRACE_DBG("%s", "No media");
@@ -3490,20 +3464,20 @@ static int vcdrom_change(struct vdev_type *vdt, char *p, const char *name)
PRINT_ERROR("File path \"%s\" is not "
"absolute", file_name);
res = -EINVAL;
goto out;
goto out_resume;
} else
virt_dev->cdrom_empty = 0;
old_fn = virt_dev->file_name;
if (!virt_dev->cdrom_empty) {
len = strlen(file_name) + 1;
fn = kmalloc(len, GFP_KERNEL);
int len = strlen(file_name) + 1;
char *fn = kmalloc(len, GFP_KERNEL);
if (fn == NULL) {
TRACE(TRACE_OUT_OF_MEM, "%s",
"Allocation of file_name failed");
res = -ENOMEM;
goto out;
goto out_resume;
}
strncpy(fn, file_name, len);
@@ -3518,15 +3492,11 @@ static int vcdrom_change(struct vdev_type *vdt, char *p, const char *name)
virt_dev->file_name = NULL;
}
res = scst_suspend_activity(true);
if (res != 0)
goto out_free;
if (virt_dev->prevent_allow_medium_removal) {
PRINT_ERROR("Prevent medium removal for "
"virtual device with name %s", name);
"virtual device with name %s", virt_dev->name);
res = -EINVAL;
goto out_free_resume;
goto out_free;
}
virt_dev->file_size = err;
@@ -3556,52 +3526,40 @@ out_resume:
scst_resume_activity();
out:
TRACE_EXIT_RES(res);
return res;
out_free:
kfree(virt_dev->file_name);
virt_dev->file_name = old_fn;
kfree(fn);
goto out;
out_free_resume:
virt_dev->file_name = old_fn;
kfree(fn);
goto out_resume;
}
static int vcdrom_parse_cmd(struct vdev_type *vdt, char *p, int *action)
{
int res = -EINVAL;
TRACE_ENTRY();
if (!strncmp("change", p, 6)) {
res = 6;
p += res;
*action = VCDROM_ACTION_CHANGE;
}
TRACE_EXIT_RES(res);
return res;
}
/* scst_vdisk_mutex supposed to be held */
static int vcdrom_perform_cmd(struct vdev_type *vdt, int action, char *p,
char *name)
{
int res = -EINVAL;
TRACE_ENTRY();
if (action == VCDROM_ACTION_CHANGE)
res = vcdrom_change(vdt, p, name);
TRACE_EXIT_RES(res);
return res;
}
#ifndef CONFIG_SCST_PROC
static ssize_t vcdrom_sysfs_filename_store(struct kobject *kobj,
struct kobj_attribute *attr, const char *buf, size_t count)
{
int res;
struct scst_device *dev;
struct scst_vdisk_dev *virt_dev;
TRACE_ENTRY();
dev = container_of(kobj, struct scst_device, dev_kobj);
virt_dev = (struct scst_vdisk_dev *)dev->dh_priv;
res = vcdrom_change(virt_dev, buf, count);
if (res != 0)
goto out;
res = count;
out:
TRACE_EXIT_RES(res);
return res;
}
static int vdisk_mgmt_cmd(const char *buffer, int length,
struct vdev_type *vdt)
{
@@ -3618,6 +3576,7 @@ static int vdisk_mgmt_cmd(const char *buffer, int length,
if (i_buf == NULL) {
PRINT_ERROR("Unable to alloc intermediate buffer with size %d",
length+1);
res = -ENOMEM;
goto out;
}
@@ -3639,15 +3598,9 @@ static int vdisk_mgmt_cmd(const char *buffer, int length,
p += 5;
action = VDEV_ACTION_CLOSE;
} else {
int n;
n = vdt->vfns.parse_cmd(vdt, p, &action);
if (n > 0)
p += n;
else {
PRINT_ERROR("Unknown action \"%s\"", p);
res = -EINVAL;
goto out_up;
}
PRINT_ERROR("Unknown action \"%s\"", p);
res = -EINVAL;
goto out_up;
}
if (!isspace(*p)) {
@@ -3681,11 +3634,9 @@ static int vdisk_mgmt_cmd(const char *buffer, int length,
res = vdt->vfns.vdev_close(vdt, name);
if (res != 0)
goto out_up;
} else {
res = vdt->vfns.perform_cmd(vdt, action, p, name);
if (res != 0)
goto out_up;
}
} else
sBUG();
res = length;
out_up:
@@ -3880,20 +3831,12 @@ static ssize_t vdisk_sysfs_resync_size_store(struct kobject *kobj,
dev = container_of(kobj, struct scst_device, dev_kobj);
virt_dev = (struct scst_vdisk_dev *)dev->dh_priv;
if (mutex_lock_interruptible(&scst_vdisk_mutex) != 0) {
res = -EINTR;
goto out;
}
res = __vdisk_resync_size(virt_dev);
res = vdisk_resync_size(virt_dev);
if (res != 0)
goto out_unlock;
goto out;
res = count;
out_unlock:
mutex_unlock(&scst_vdisk_mutex);
out:
TRACE_EXIT_RES(res);
return res;
@@ -3993,6 +3936,7 @@ static int vdisk_proc_mgmt_cmd(const char *buffer, int length,
if (i_buf == NULL) {
PRINT_ERROR("Unable to alloc intermediate buffer with size %d",
length+1);
res = -ENOMEM;
goto out;
}
@@ -4226,7 +4170,7 @@ static int vdisk_proc_mgmt_cmd(const char *buffer, int length,
goto out_up;
}
res = __vdisk_resync_size(virt_dev);
res = vdisk_resync_size(virt_dev);
if (res != 0)
goto out_up;
}
@@ -4296,6 +4240,26 @@ out:
return res;
}
/* scst_vdisk_mutex supposed to be held */
static int vcdrom_proc_change(struct vdev_type *vdt, char *p, const char *name)
{
struct scst_vdisk_dev *virt_dev;
int res;
virt_dev = vdt->vfns.vdev_find(name);
if (virt_dev == NULL) {
PRINT_ERROR("Virtual device with name "
"%s not found", name);
res = -EINVAL;
goto out;
}
res = vcdrom_change(virt_dev, p, strlen(p) + 1);
out:
return res;
}
static int vcdrom_proc_mgmt_cmd(const char *buffer, int length,
struct scst_dev_type *dev_type)
{
@@ -4312,6 +4276,7 @@ static int vcdrom_proc_mgmt_cmd(const char *buffer, int length,
if (i_buf == NULL) {
PRINT_ERROR("Unable to alloc intermediate buffer with size %d",
length+1);
res = -ENOMEM;
goto out;
}
@@ -4365,7 +4330,7 @@ static int vcdrom_proc_mgmt_cmd(const char *buffer, int length,
goto out_up;
} else if (action == 1) {
/* change */
res = vcdrom_change(&vcdrom_type.parent_vdt, p, name);
res = vcdrom_proc_change(&vcdrom_type.parent_vdt, p, name);
if (res != 0)
goto out_up;
} else {
@@ -4558,7 +4523,7 @@ static void __init init_fileio_type(void)
vdt->vdt_name = "FILEIO";
vdt->help_string = "Usage:\n"
" echo \"open|close|resync_size NAME [FILE_NAME "
" echo \"open|close NAME [FILE_NAME "
"[BLOCK_SIZE] [WRITE_THROUGH] [READ_ONLY] [O_DIRECT] "
"[NV_CACHE] [REMOVABLE]]\" >mgmt\n";
@@ -4566,8 +4531,6 @@ static void __init init_fileio_type(void)
vdt->vfns.vdev_add = vdisk_add;
vdt->vfns.vdev_find = vdisk_find;
vdt->vfns.parse_cmd = vdisk_parse_cmd;
vdt->vfns.perform_cmd = vdisk_perform_cmd;
vdt->vfns.parse_option = vdisk_fileio_parse_option;
vdt->vfns.pre_register = vdisk_fileio_pre_register;
@@ -4582,7 +4545,7 @@ static void __init init_blockio_type(void)
vdt->vdt_name = "BLOCKIO";
vdt->help_string = "Usage:\n"
" echo \"open|close|resync_size NAME [DEVICE_NAME "
" echo \"open|close NAME [DEVICE_NAME "
"[BLOCK_SIZE] [READ_ONLY] [REMOVABLE]]\" >mgmt\n";
blockio_type.parent_vdt_vfns = vdt->vfns;
@@ -4590,8 +4553,6 @@ static void __init init_blockio_type(void)
vdt->vfns.vdev_init = vdisk_blockio_init;
vdt->vfns.vdev_add = vdisk_add;
vdt->vfns.vdev_find = vdisk_find;
vdt->vfns.parse_cmd = vdisk_parse_cmd;
vdt->vfns.perform_cmd = vdisk_perform_cmd;
vdt->vfns.parse_option = vdisk_parse_option;
vdt->vfns.pre_register = vdisk_blockio_pre_register;
@@ -4613,9 +4574,7 @@ static void __init init_nullio_type(void)
vdt->vfns.vdev_init = vdisk_nullio_init;
vdt->vfns.vdev_add = vdisk_add;
vdt->vfns.vdev_find = vdisk_find;
vdt->vfns.parse_cmd = vdisk_parse_cmd;
vdt->vfns.parse_option = vdisk_parse_option;
vdt->vfns.perform_cmd = vdisk_perform_cmd;
return;
}
@@ -4634,8 +4593,6 @@ static void __init init_vcdrom_type(void)
vdt->vfns.vdev_init = vcdrom_init;
vdt->vfns.vdev_add = vcdrom_add;
vdt->vfns.vdev_find = vcdrom_find;
vdt->vfns.parse_cmd = vcdrom_parse_cmd;
vdt->vfns.perform_cmd = vcdrom_perform_cmd;
vdt->vfns.pre_register = vcdrom_pre_register;
return;

View File

@@ -1334,7 +1334,6 @@ out:
return res;
}
/* Called under scst_mutex and suspended activity */
void scst_free_device(struct scst_device *dev)
{
TRACE_ENTRY();
@@ -2377,7 +2376,7 @@ void scst_free_session(struct scst_session *sess)
mutex_unlock(&scst_mutex);
scst_sess_sysfs_put(sess);
scst_sess_sysfs_put(sess); /* must not be called under scst_mutex */
TRACE_EXIT();
return;

View File

@@ -468,7 +468,7 @@ out_resume_free:
scst_resume_activity();
out_free_tgt_err:
scst_tgt_sysfs_put(tgt);
scst_tgt_sysfs_put(tgt); /* must not be called under scst_mutex */
tgt = NULL;
out_err:
@@ -544,7 +544,7 @@ again:
del_timer_sync(&tgt->retry_timer);
scst_tgt_sysfs_put(tgt);
scst_tgt_sysfs_put(tgt); /* must not be called under scst_mutex */
PRINT_INFO("Target %p for template %s unregistered successfully",
tgt, vtt->name);
@@ -805,8 +805,10 @@ out_free:
#endif
out_free_dev:
scst_device_sysfs_put(dev);
goto out_up;
mutex_unlock(&scst_mutex);
scst_resume_activity();
scst_device_sysfs_put(dev); /* must not be called under scst_mutex */
goto out_err;
}
static void scst_unregister_device(struct scsi_device *scsidp)
@@ -828,7 +830,7 @@ static void scst_unregister_device(struct scsi_device *scsidp)
}
if (dev == NULL) {
PRINT_ERROR("%s", "Target device not found");
goto out_unblock;
goto out_resume;
}
list_del(&dev->dev_list_entry);
@@ -844,18 +846,23 @@ static void scst_unregister_device(struct scsi_device *scsidp)
put_disk(dev->rq_disk);
#endif
scst_device_sysfs_put(dev);
mutex_unlock(&scst_mutex);
scst_resume_activity();
scst_device_sysfs_put(dev); /* must not be called under scst_mutex */
PRINT_INFO("Detached from scsi%d, channel %d, id %d, lun %d, type %d",
scsidp->host->host_no, scsidp->channel, scsidp->id,
scsidp->lun, scsidp->type);
out_unblock:
mutex_unlock(&scst_mutex);
scst_resume_activity();
out:
TRACE_EXIT();
return;
out_resume:
mutex_unlock(&scst_mutex);
scst_resume_activity();
goto out;
}
static int scst_dev_handler_check(struct scst_dev_type *dev_handler)
@@ -970,8 +977,10 @@ out_free_del:
list_del(&dev->dev_list_entry);
out_release:
scst_device_sysfs_put(dev);
goto out_up;
mutex_unlock(&scst_mutex);
scst_resume_activity();
scst_device_sysfs_put(dev); /* must not be called under scst_mutex */
goto out;
}
EXPORT_SYMBOL(scst_register_virtual_device);
@@ -1009,12 +1018,12 @@ void scst_unregister_virtual_device(int id)
PRINT_INFO("Detached from virtual device %s (id %d)",
dev->virt_name, dev->virt_id);
scst_device_sysfs_put(dev);
out_unblock:
mutex_unlock(&scst_mutex);
scst_resume_activity();
scst_device_sysfs_put(dev); /* must not be called under scst_mutex */
TRACE_EXIT();
return;
}

View File

@@ -484,6 +484,10 @@ out_nomem:
goto out;
}
/*
* Must not be called under scst_mutex or there can be a deadlock with
* tgt_attr_rwsem
*/
void scst_tgt_sysfs_put(struct scst_tgt *tgt)
{
if (tgt->tgt_kobj_initialized) {
@@ -635,10 +639,17 @@ static ssize_t scst_dev_attr_show(struct kobject *kobj, struct attribute *attr,
goto out;
}
if (mutex_lock_interruptible(&dev->dev_sysfs_mutex) != 0) {
res = -EINTR;
goto out;
}
kobj_attr = container_of(attr, struct kobj_attribute, attr);
res = kobj_attr->show(kobj, kobj_attr, buf);
mutex_unlock(&dev->dev_sysfs_mutex);
up_read(&dev->dev_attr_rwsem);
out:
@@ -659,10 +670,17 @@ static ssize_t scst_dev_attr_store(struct kobject *kobj, struct attribute *attr,
goto out;
}
if (mutex_lock_interruptible(&dev->dev_sysfs_mutex) != 0) {
res = -EINTR;
goto out;
}
kobj_attr = container_of(attr, struct kobj_attribute, attr);
res = kobj_attr->store(kobj, kobj_attr, buf, count);
mutex_unlock(&dev->dev_sysfs_mutex);
up_read(&dev->dev_attr_rwsem);
out:
@@ -687,6 +705,7 @@ int scst_create_device_sysfs(struct scst_device *dev)
TRACE_ENTRY();
init_rwsem(&dev->dev_attr_rwsem);
mutex_init(&dev->dev_sysfs_mutex);
dev->dev_kobj_initialized = 1;
@@ -726,6 +745,10 @@ out:
return retval;
}
/*
* Must not be called under scst_mutex or there can be a deadlock with
* dev_attr_rwsem
*/
void scst_device_sysfs_put(struct scst_device *dev)
{
TRACE_ENTRY();
@@ -937,6 +960,10 @@ out_free:
return retval;
}
/*
* Must not be called under scst_mutex or there can be a deadlock with
* sess_attr_rwsem
*/
void scst_sess_sysfs_put(struct scst_session *sess)
{
TRACE_ENTRY();