[PATCH v6 0/2] blktrace: Fix deadlock problem

[PATCH v6 0/2] blktrace: Fix deadlock problem

[Waiman Long]

 v6:
  - Add a second patch to rename the bd_fsfreeze_mutex to
    bd_fsfreeze_blktrace_mutex.

 v5:
  - Overload the bd_fsfreeze_mutex in block_device structure for
    blktrace protection.

 v4:
  - Use blktrace_mutex in blk_trace_ioctl() as well.

 v3:
  - Use a global blktrace_mutex to serialize sysfs attribute accesses
    instead of the bd_mutex.

 v2:
  - Use READ_ONCE() and smp_store_mb() to read and write bd_deleting.
  - Check for signal in the mutex_trylock loops.
  - Use usleep() instead of schedule() for RT tasks.

This patchset fixes a potential blktrace deadlock problem between
block device deletion and sysfs operations.

Waiman Long (2):
  blktrace: Fix potentail deadlock between delete & sysfs ops
  block_dev: Rename bd_fsfreeze_mutex

 fs/block_dev.c          | 14 +++++++-------
 fs/gfs2/ops_fstype.c    |  6 +++---
 fs/nilfs2/super.c       |  6 +++---
 fs/super.c              |  6 +++---
 include/linux/fs.h      |  5 +++--
 kernel/trace/blktrace.c | 26 ++++++++++++++++++++------
 6 files changed, 39 insertions(+), 24 deletions(-)

-- 
1.8.3.1

[Cluster-devel] [PATCH v6 0/2] blktrace: Fix deadlock problem

[Waiman Long]

 v6:
  - Add a second patch to rename the bd_fsfreeze_mutex to
    bd_fsfreeze_blktrace_mutex.

 v5:
  - Overload the bd_fsfreeze_mutex in block_device structure for
    blktrace protection.

 v4:
  - Use blktrace_mutex in blk_trace_ioctl() as well.

 v3:
  - Use a global blktrace_mutex to serialize sysfs attribute accesses
    instead of the bd_mutex.

 v2:
  - Use READ_ONCE() and smp_store_mb() to read and write bd_deleting.
  - Check for signal in the mutex_trylock loops.
  - Use usleep() instead of schedule() for RT tasks.

This patchset fixes a potential blktrace deadlock problem between
block device deletion and sysfs operations.

Waiman Long (2):
  blktrace: Fix potentail deadlock between delete & sysfs ops
  block_dev: Rename bd_fsfreeze_mutex

 fs/block_dev.c          | 14 +++++++-------
 fs/gfs2/ops_fstype.c    |  6 +++---
 fs/nilfs2/super.c       |  6 +++---
 fs/super.c              |  6 +++---
 include/linux/fs.h      |  5 +++--
 kernel/trace/blktrace.c | 26 ++++++++++++++++++++------
 6 files changed, 39 insertions(+), 24 deletions(-)

-- 
1.8.3.1

[PATCH v6 1/2] blktrace: Fix potentail deadlock between delete & sysfs ops

[Waiman Long]

The lockdep code had reported the following unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(s_active#228);
                               lock(&bdev->bd_mutex/1);
                               lock(s_active#228);
  lock(&bdev->bd_mutex);

 *** DEADLOCK ***

The deadlock may happen when one task (CPU1) is trying to delete a
partition in a block device and another task (CPU0) is accessing
tracing sysfs file (e.g. /sys/block/dm-1/trace/act_mask) in that
partition.

The s_active isn't an actual lock. It is a reference count (kn->count)
on the sysfs (kernfs) file. Removal of a sysfs file, however, require
a wait until all the references are gone. The reference count is
treated like a rwsem using lockdep instrumentation code.

The fact that a thread is in the sysfs callback method or in the
ioctl call means there is a reference to the opended sysfs or device
file. That should prevent the underlying block structure from being
removed.

Instead of using bd_mutex in the block_device structure, the other
bd_fsfreeze_mutex mutex in the block_device structure is now overloaded
to protect against concurrent blktrace data access in the blktrace.c
file. There is no point in adding one more mutex to the block_device
structure just for blktrace.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 include/linux/fs.h      |  2 +-
 kernel/trace/blktrace.c | 26 ++++++++++++++++++++------
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 339e737..330b572 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -448,7 +448,7 @@ struct block_device {
 
 	/* The counter of freeze processes */
 	int			bd_fsfreeze_count;
-	/* Mutex for freeze */
+	/* Mutex for freeze and blktrace */
 	struct mutex		bd_fsfreeze_mutex;
 } __randomize_layout;
 
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 2a685b4..7cd5d1d 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -648,6 +648,20 @@ int blk_trace_startstop(struct request_queue *q, int start)
 }
 EXPORT_SYMBOL_GPL(blk_trace_startstop);
 
+/*
+ * When reading or writing the blktrace sysfs files, the references to the
+ * opened sysfs or device files should prevent the underlying block device
+ * from being removed. So no further delete protection is really needed.
+ *
+ * Protection from multiple readers and writers accessing blktrace data
+ * concurrently is still required. The bd_mutex was used for this purpose.
+ * That could lead to deadlock with concurrent block device deletion and
+ * sysfs access. Instead, the block device bd_fsfreeze_mutex is now
+ * overloaded for blktrace data protection. Like freeze/thaw, blktrace
+ * functionality is not frequently used. There is no point in adding
+ * one more mutex to the block_device structure just for blktrace.
+ */
+
 /**
  * blk_trace_ioctl: - handle the ioctls associated with tracing
  * @bdev:	the block device
@@ -665,7 +679,7 @@ int blk_trace_ioctl(struct block_device *bdev, unsigned cmd, char __user *arg)
 	if (!q)
 		return -ENXIO;
 
-	mutex_lock(&bdev->bd_mutex);
+	mutex_lock(&bdev->bd_fsfreeze_mutex);
 
 	switch (cmd) {
 	case BLKTRACESETUP:
@@ -691,7 +705,7 @@ int blk_trace_ioctl(struct block_device *bdev, unsigned cmd, char __user *arg)
 		break;
 	}
 
-	mutex_unlock(&bdev->bd_mutex);
+	mutex_unlock(&bdev->bd_fsfreeze_mutex);
 	return ret;
 }
 
@@ -1727,7 +1741,7 @@ static ssize_t sysfs_blk_trace_attr_show(struct device *dev,
 	if (q == NULL)
 		goto out_bdput;
 
-	mutex_lock(&bdev->bd_mutex);
+	mutex_lock(&bdev->bd_fsfreeze_mutex);
 
 	if (attr == &dev_attr_enable) {
 		ret = sprintf(buf, "%u\n", !!q->blk_trace);
@@ -1746,7 +1760,7 @@ static ssize_t sysfs_blk_trace_attr_show(struct device *dev,
 		ret = sprintf(buf, "%llu\n", q->blk_trace->end_lba);
 
 out_unlock_bdev:
-	mutex_unlock(&bdev->bd_mutex);
+	mutex_unlock(&bdev->bd_fsfreeze_mutex);
 out_bdput:
 	bdput(bdev);
 out:
@@ -1788,7 +1802,7 @@ static ssize_t sysfs_blk_trace_attr_store(struct device *dev,
 	if (q == NULL)
 		goto out_bdput;
 
-	mutex_lock(&bdev->bd_mutex);
+	mutex_lock(&bdev->bd_fsfreeze_mutex);
 
 	if (attr == &dev_attr_enable) {
 		if (value)
@@ -1814,7 +1828,7 @@ static ssize_t sysfs_blk_trace_attr_store(struct device *dev,
 	}
 
 out_unlock_bdev:
-	mutex_unlock(&bdev->bd_mutex);
+	mutex_unlock(&bdev->bd_fsfreeze_mutex);
 out_bdput:
 	bdput(bdev);
 out:
-- 
1.8.3.1

[Cluster-devel] [PATCH v6 1/2] blktrace: Fix potentail deadlock between delete & sysfs ops

[Waiman Long]

The lockdep code had reported the following unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(s_active#228);
                               lock(&bdev->bd_mutex/1);
                               lock(s_active#228);
  lock(&bdev->bd_mutex);

 *** DEADLOCK ***

The deadlock may happen when one task (CPU1) is trying to delete a
partition in a block device and another task (CPU0) is accessing
tracing sysfs file (e.g. /sys/block/dm-1/trace/act_mask) in that
partition.

The s_active isn't an actual lock. It is a reference count (kn->count)
on the sysfs (kernfs) file. Removal of a sysfs file, however, require
a wait until all the references are gone. The reference count is
treated like a rwsem using lockdep instrumentation code.

The fact that a thread is in the sysfs callback method or in the
ioctl call means there is a reference to the opended sysfs or device
file. That should prevent the underlying block structure from being
removed.

Instead of using bd_mutex in the block_device structure, the other
bd_fsfreeze_mutex mutex in the block_device structure is now overloaded
to protect against concurrent blktrace data access in the blktrace.c
file. There is no point in adding one more mutex to the block_device
structure just for blktrace.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 include/linux/fs.h      |  2 +-
 kernel/trace/blktrace.c | 26 ++++++++++++++++++++------
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 339e737..330b572 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -448,7 +448,7 @@ struct block_device {
 
 	/* The counter of freeze processes */
 	int			bd_fsfreeze_count;
-	/* Mutex for freeze */
+	/* Mutex for freeze and blktrace */
 	struct mutex		bd_fsfreeze_mutex;
 } __randomize_layout;
 
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 2a685b4..7cd5d1d 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -648,6 +648,20 @@ int blk_trace_startstop(struct request_queue *q, int start)
 }
 EXPORT_SYMBOL_GPL(blk_trace_startstop);
 
+/*
+ * When reading or writing the blktrace sysfs files, the references to the
+ * opened sysfs or device files should prevent the underlying block device
+ * from being removed. So no further delete protection is really needed.
+ *
+ * Protection from multiple readers and writers accessing blktrace data
+ * concurrently is still required. The bd_mutex was used for this purpose.
+ * That could lead to deadlock with concurrent block device deletion and
+ * sysfs access. Instead, the block device bd_fsfreeze_mutex is now
+ * overloaded for blktrace data protection. Like freeze/thaw, blktrace
+ * functionality is not frequently used. There is no point in adding
+ * one more mutex to the block_device structure just for blktrace.
+ */
+
 /**
  * blk_trace_ioctl: - handle the ioctls associated with tracing
  * @bdev:	the block device
@@ -665,7 +679,7 @@ int blk_trace_ioctl(struct block_device *bdev, unsigned cmd, char __user *arg)
 	if (!q)
 		return -ENXIO;
 
-	mutex_lock(&bdev->bd_mutex);
+	mutex_lock(&bdev->bd_fsfreeze_mutex);
 
 	switch (cmd) {
 	case BLKTRACESETUP:
@@ -691,7 +705,7 @@ int blk_trace_ioctl(struct block_device *bdev, unsigned cmd, char __user *arg)
 		break;
 	}
 
-	mutex_unlock(&bdev->bd_mutex);
+	mutex_unlock(&bdev->bd_fsfreeze_mutex);
 	return ret;
 }
 
@@ -1727,7 +1741,7 @@ static ssize_t sysfs_blk_trace_attr_show(struct device *dev,
 	if (q == NULL)
 		goto out_bdput;
 
-	mutex_lock(&bdev->bd_mutex);
+	mutex_lock(&bdev->bd_fsfreeze_mutex);
 
 	if (attr == &dev_attr_enable) {
 		ret = sprintf(buf, "%u\n", !!q->blk_trace);
@@ -1746,7 +1760,7 @@ static ssize_t sysfs_blk_trace_attr_show(struct device *dev,
 		ret = sprintf(buf, "%llu\n", q->blk_trace->end_lba);
 
 out_unlock_bdev:
-	mutex_unlock(&bdev->bd_mutex);
+	mutex_unlock(&bdev->bd_fsfreeze_mutex);
 out_bdput:
 	bdput(bdev);
 out:
@@ -1788,7 +1802,7 @@ static ssize_t sysfs_blk_trace_attr_store(struct device *dev,
 	if (q == NULL)
 		goto out_bdput;
 
-	mutex_lock(&bdev->bd_mutex);
+	mutex_lock(&bdev->bd_fsfreeze_mutex);
 
 	if (attr == &dev_attr_enable) {
 		if (value)
@@ -1814,7 +1828,7 @@ static ssize_t sysfs_blk_trace_attr_store(struct device *dev,
 	}
 
 out_unlock_bdev:
-	mutex_unlock(&bdev->bd_mutex);
+	mutex_unlock(&bdev->bd_fsfreeze_mutex);
 out_bdput:
 	bdput(bdev);
 out:
-- 
1.8.3.1

[PATCH v6 2/2] block_dev: Rename bd_fsfreeze_mutex

[Waiman Long]

As the bd_fsfreeze_mutex is used by the blktrace subsystem as well,
it is now renamed to bd_fsfreeze_blktrace_mutex to better reflect
its purpose.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 fs/block_dev.c          | 14 +++++++-------
 fs/gfs2/ops_fstype.c    |  6 +++---
 fs/nilfs2/super.c       |  6 +++---
 fs/super.c              |  6 +++---
 include/linux/fs.h      |  5 +++--
 kernel/trace/blktrace.c | 14 +++++++-------
 6 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 93d088f..3dea006 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -504,7 +504,7 @@ struct super_block *freeze_bdev(struct block_device *bdev)
 	struct super_block *sb;
 	int error = 0;
 
-	mutex_lock(&bdev->bd_fsfreeze_mutex);
+	mutex_lock(&bdev->bd_fsfreeze_blktrace_mutex);
 	if (++bdev->bd_fsfreeze_count > 1) {
 		/*
 		 * We don't even need to grab a reference - the first call
@@ -514,7 +514,7 @@ struct super_block *freeze_bdev(struct block_device *bdev)
 		sb = get_super(bdev);
 		if (sb)
 			drop_super(sb);
-		mutex_unlock(&bdev->bd_fsfreeze_mutex);
+		mutex_unlock(&bdev->bd_fsfreeze_blktrace_mutex);
 		return sb;
 	}
 
@@ -528,13 +528,13 @@ struct super_block *freeze_bdev(struct block_device *bdev)
 	if (error) {
 		deactivate_super(sb);
 		bdev->bd_fsfreeze_count--;
-		mutex_unlock(&bdev->bd_fsfreeze_mutex);
+		mutex_unlock(&bdev->bd_fsfreeze_blktrace_mutex);
 		return ERR_PTR(error);
 	}
 	deactivate_super(sb);
  out:
 	sync_blockdev(bdev);
-	mutex_unlock(&bdev->bd_fsfreeze_mutex);
+	mutex_unlock(&bdev->bd_fsfreeze_blktrace_mutex);
 	return sb;	/* thaw_bdev releases s->s_umount */
 }
 EXPORT_SYMBOL(freeze_bdev);
@@ -550,7 +550,7 @@ int thaw_bdev(struct block_device *bdev, struct super_block *sb)
 {
 	int error = -EINVAL;
 
-	mutex_lock(&bdev->bd_fsfreeze_mutex);
+	mutex_lock(&bdev->bd_fsfreeze_blktrace_mutex);
 	if (!bdev->bd_fsfreeze_count)
 		goto out;
 
@@ -568,7 +568,7 @@ int thaw_bdev(struct block_device *bdev, struct super_block *sb)
 	if (error)
 		bdev->bd_fsfreeze_count++;
 out:
-	mutex_unlock(&bdev->bd_fsfreeze_mutex);
+	mutex_unlock(&bdev->bd_fsfreeze_blktrace_mutex);
 	return error;
 }
 EXPORT_SYMBOL(thaw_bdev);
@@ -767,7 +767,7 @@ static void init_once(void *foo)
 	bdev->bd_bdi = &noop_backing_dev_info;
 	inode_init_once(&ei->vfs_inode);
 	/* Initialize mutex for freeze. */
-	mutex_init(&bdev->bd_fsfreeze_mutex);
+	mutex_init(&bdev->bd_fsfreeze_blktrace_mutex);
 }
 
 static void bdev_evict_inode(struct inode *inode)
diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
index a3711f5..5664905 100644
--- a/fs/gfs2/ops_fstype.c
+++ b/fs/gfs2/ops_fstype.c
@@ -1269,14 +1269,14 @@ static struct dentry *gfs2_mount(struct file_system_type *fs_type, int flags,
 	 * will protect the lockfs code from trying to start a snapshot
 	 * while we are mounting
 	 */
-	mutex_lock(&bdev->bd_fsfreeze_mutex);
+	mutex_lock(&bdev->bd_fsfreeze_blktrace_mutex);
 	if (bdev->bd_fsfreeze_count > 0) {
-		mutex_unlock(&bdev->bd_fsfreeze_mutex);
+		mutex_unlock(&bdev->bd_fsfreeze_blktrace_mutex);
 		error = -EBUSY;
 		goto error_bdev;
 	}
 	s = sget(fs_type, test_gfs2_super, set_gfs2_super, flags, bdev);
-	mutex_unlock(&bdev->bd_fsfreeze_mutex);
+	mutex_unlock(&bdev->bd_fsfreeze_blktrace_mutex);
 	error = PTR_ERR(s);
 	if (IS_ERR(s))
 		goto error_bdev;
diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
index 4fc018d..931b455 100644
--- a/fs/nilfs2/super.c
+++ b/fs/nilfs2/super.c
@@ -1306,15 +1306,15 @@ static int nilfs_test_bdev_super(struct super_block *s, void *data)
 	 * will protect the lockfs code from trying to start a snapshot
 	 * while we are mounting
 	 */
-	mutex_lock(&sd.bdev->bd_fsfreeze_mutex);
+	mutex_lock(&sd.bdev->bd_fsfreeze_blktrace_mutex);
 	if (sd.bdev->bd_fsfreeze_count > 0) {
-		mutex_unlock(&sd.bdev->bd_fsfreeze_mutex);
+		mutex_unlock(&sd.bdev->bd_fsfreeze_blktrace_mutex);
 		err = -EBUSY;
 		goto failed;
 	}
 	s = sget(fs_type, nilfs_test_bdev_super, nilfs_set_bdev_super, flags,
 		 sd.bdev);
-	mutex_unlock(&sd.bdev->bd_fsfreeze_mutex);
+	mutex_unlock(&sd.bdev->bd_fsfreeze_blktrace_mutex);
 	if (IS_ERR(s)) {
 		err = PTR_ERR(s);
 		goto failed;
diff --git a/fs/super.c b/fs/super.c
index 166c4ee..079890f 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1083,15 +1083,15 @@ struct dentry *mount_bdev(struct file_system_type *fs_type,
 	 * will protect the lockfs code from trying to start a snapshot
 	 * while we are mounting
 	 */
-	mutex_lock(&bdev->bd_fsfreeze_mutex);
+	mutex_lock(&bdev->bd_fsfreeze_blktrace_mutex);
 	if (bdev->bd_fsfreeze_count > 0) {
-		mutex_unlock(&bdev->bd_fsfreeze_mutex);
+		mutex_unlock(&bdev->bd_fsfreeze_blktrace_mutex);
 		error = -EBUSY;
 		goto error_bdev;
 	}
 	s = sget(fs_type, test_bdev_super, set_bdev_super, flags | SB_NOSEC,
 		 bdev);
-	mutex_unlock(&bdev->bd_fsfreeze_mutex);
+	mutex_unlock(&bdev->bd_fsfreeze_blktrace_mutex);
 	if (IS_ERR(s))
 		goto error_s;
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 330b572..16c4297 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -448,8 +448,9 @@ struct block_device {
 
 	/* The counter of freeze processes */
 	int			bd_fsfreeze_count;
-	/* Mutex for freeze and blktrace */
-	struct mutex		bd_fsfreeze_mutex;
+
+	/* Mutex for both freeze and blktrace */
+	struct mutex		bd_fsfreeze_blktrace_mutex;
 } __randomize_layout;
 
 /*
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 7cd5d1d..463a470 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -656,7 +656,7 @@ int blk_trace_startstop(struct request_queue *q, int start)
  * Protection from multiple readers and writers accessing blktrace data
  * concurrently is still required. The bd_mutex was used for this purpose.
  * That could lead to deadlock with concurrent block device deletion and
- * sysfs access. Instead, the block device bd_fsfreeze_mutex is now
+ * sysfs access. Instead, the block device bd_fsfreeze_blktrace_mutex is now
  * overloaded for blktrace data protection. Like freeze/thaw, blktrace
  * functionality is not frequently used. There is no point in adding
  * one more mutex to the block_device structure just for blktrace.
@@ -679,7 +679,7 @@ int blk_trace_ioctl(struct block_device *bdev, unsigned cmd, char __user *arg)
 	if (!q)
 		return -ENXIO;
 
-	mutex_lock(&bdev->bd_fsfreeze_mutex);
+	mutex_lock(&bdev->bd_fsfreeze_blktrace_mutex);
 
 	switch (cmd) {
 	case BLKTRACESETUP:
@@ -705,7 +705,7 @@ int blk_trace_ioctl(struct block_device *bdev, unsigned cmd, char __user *arg)
 		break;
 	}
 
-	mutex_unlock(&bdev->bd_fsfreeze_mutex);
+	mutex_unlock(&bdev->bd_fsfreeze_blktrace_mutex);
 	return ret;
 }
 
@@ -1741,7 +1741,7 @@ static ssize_t sysfs_blk_trace_attr_show(struct device *dev,
 	if (q == NULL)
 		goto out_bdput;
 
-	mutex_lock(&bdev->bd_fsfreeze_mutex);
+	mutex_lock(&bdev->bd_fsfreeze_blktrace_mutex);
 
 	if (attr == &dev_attr_enable) {
 		ret = sprintf(buf, "%u\n", !!q->blk_trace);
@@ -1760,7 +1760,7 @@ static ssize_t sysfs_blk_trace_attr_show(struct device *dev,
 		ret = sprintf(buf, "%llu\n", q->blk_trace->end_lba);
 
 out_unlock_bdev:
-	mutex_unlock(&bdev->bd_fsfreeze_mutex);
+	mutex_unlock(&bdev->bd_fsfreeze_blktrace_mutex);
 out_bdput:
 	bdput(bdev);
 out:
@@ -1802,7 +1802,7 @@ static ssize_t sysfs_blk_trace_attr_store(struct device *dev,
 	if (q == NULL)
 		goto out_bdput;
 
-	mutex_lock(&bdev->bd_fsfreeze_mutex);
+	mutex_lock(&bdev->bd_fsfreeze_blktrace_mutex);
 
 	if (attr == &dev_attr_enable) {
 		if (value)
@@ -1828,7 +1828,7 @@ static ssize_t sysfs_blk_trace_attr_store(struct device *dev,
 	}
 
 out_unlock_bdev:
-	mutex_unlock(&bdev->bd_fsfreeze_mutex);
+	mutex_unlock(&bdev->bd_fsfreeze_blktrace_mutex);
 out_bdput:
 	bdput(bdev);
 out:
-- 
1.8.3.1

[Cluster-devel] [PATCH v6 2/2] block_dev: Rename bd_fsfreeze_mutex

[Waiman Long]

As the bd_fsfreeze_mutex is used by the blktrace subsystem as well,
it is now renamed to bd_fsfreeze_blktrace_mutex to better reflect
its purpose.

Signed-off-by: Waiman Long <longman@redhat.com>
---
 fs/block_dev.c          | 14 +++++++-------
 fs/gfs2/ops_fstype.c    |  6 +++---
 fs/nilfs2/super.c       |  6 +++---
 fs/super.c              |  6 +++---
 include/linux/fs.h      |  5 +++--
 kernel/trace/blktrace.c | 14 +++++++-------
 6 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 93d088f..3dea006 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -504,7 +504,7 @@ struct super_block *freeze_bdev(struct block_device *bdev)
 	struct super_block *sb;
 	int error = 0;
 
-	mutex_lock(&bdev->bd_fsfreeze_mutex);
+	mutex_lock(&bdev->bd_fsfreeze_blktrace_mutex);
 	if (++bdev->bd_fsfreeze_count > 1) {
 		/*
 		 * We don't even need to grab a reference - the first call
@@ -514,7 +514,7 @@ struct super_block *freeze_bdev(struct block_device *bdev)
 		sb = get_super(bdev);
 		if (sb)
 			drop_super(sb);
-		mutex_unlock(&bdev->bd_fsfreeze_mutex);
+		mutex_unlock(&bdev->bd_fsfreeze_blktrace_mutex);
 		return sb;
 	}
 
@@ -528,13 +528,13 @@ struct super_block *freeze_bdev(struct block_device *bdev)
 	if (error) {
 		deactivate_super(sb);
 		bdev->bd_fsfreeze_count--;
-		mutex_unlock(&bdev->bd_fsfreeze_mutex);
+		mutex_unlock(&bdev->bd_fsfreeze_blktrace_mutex);
 		return ERR_PTR(error);
 	}
 	deactivate_super(sb);
  out:
 	sync_blockdev(bdev);
-	mutex_unlock(&bdev->bd_fsfreeze_mutex);
+	mutex_unlock(&bdev->bd_fsfreeze_blktrace_mutex);
 	return sb;	/* thaw_bdev releases s->s_umount */
 }
 EXPORT_SYMBOL(freeze_bdev);
@@ -550,7 +550,7 @@ int thaw_bdev(struct block_device *bdev, struct super_block *sb)
 {
 	int error = -EINVAL;
 
-	mutex_lock(&bdev->bd_fsfreeze_mutex);
+	mutex_lock(&bdev->bd_fsfreeze_blktrace_mutex);
 	if (!bdev->bd_fsfreeze_count)
 		goto out;
 
@@ -568,7 +568,7 @@ int thaw_bdev(struct block_device *bdev, struct super_block *sb)
 	if (error)
 		bdev->bd_fsfreeze_count++;
 out:
-	mutex_unlock(&bdev->bd_fsfreeze_mutex);
+	mutex_unlock(&bdev->bd_fsfreeze_blktrace_mutex);
 	return error;
 }
 EXPORT_SYMBOL(thaw_bdev);
@@ -767,7 +767,7 @@ static void init_once(void *foo)
 	bdev->bd_bdi = &noop_backing_dev_info;
 	inode_init_once(&ei->vfs_inode);
 	/* Initialize mutex for freeze. */
-	mutex_init(&bdev->bd_fsfreeze_mutex);
+	mutex_init(&bdev->bd_fsfreeze_blktrace_mutex);
 }
 
 static void bdev_evict_inode(struct inode *inode)
diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
index a3711f5..5664905 100644
--- a/fs/gfs2/ops_fstype.c
+++ b/fs/gfs2/ops_fstype.c
@@ -1269,14 +1269,14 @@ static struct dentry *gfs2_mount(struct file_system_type *fs_type, int flags,
 	 * will protect the lockfs code from trying to start a snapshot
 	 * while we are mounting
 	 */
-	mutex_lock(&bdev->bd_fsfreeze_mutex);
+	mutex_lock(&bdev->bd_fsfreeze_blktrace_mutex);
 	if (bdev->bd_fsfreeze_count > 0) {
-		mutex_unlock(&bdev->bd_fsfreeze_mutex);
+		mutex_unlock(&bdev->bd_fsfreeze_blktrace_mutex);
 		error = -EBUSY;
 		goto error_bdev;
 	}
 	s = sget(fs_type, test_gfs2_super, set_gfs2_super, flags, bdev);
-	mutex_unlock(&bdev->bd_fsfreeze_mutex);
+	mutex_unlock(&bdev->bd_fsfreeze_blktrace_mutex);
 	error = PTR_ERR(s);
 	if (IS_ERR(s))
 		goto error_bdev;
diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
index 4fc018d..931b455 100644
--- a/fs/nilfs2/super.c
+++ b/fs/nilfs2/super.c
@@ -1306,15 +1306,15 @@ static int nilfs_test_bdev_super(struct super_block *s, void *data)
 	 * will protect the lockfs code from trying to start a snapshot
 	 * while we are mounting
 	 */
-	mutex_lock(&sd.bdev->bd_fsfreeze_mutex);
+	mutex_lock(&sd.bdev->bd_fsfreeze_blktrace_mutex);
 	if (sd.bdev->bd_fsfreeze_count > 0) {
-		mutex_unlock(&sd.bdev->bd_fsfreeze_mutex);
+		mutex_unlock(&sd.bdev->bd_fsfreeze_blktrace_mutex);
 		err = -EBUSY;
 		goto failed;
 	}
 	s = sget(fs_type, nilfs_test_bdev_super, nilfs_set_bdev_super, flags,
 		 sd.bdev);
-	mutex_unlock(&sd.bdev->bd_fsfreeze_mutex);
+	mutex_unlock(&sd.bdev->bd_fsfreeze_blktrace_mutex);
 	if (IS_ERR(s)) {
 		err = PTR_ERR(s);
 		goto failed;
diff --git a/fs/super.c b/fs/super.c
index 166c4ee..079890f 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1083,15 +1083,15 @@ struct dentry *mount_bdev(struct file_system_type *fs_type,
 	 * will protect the lockfs code from trying to start a snapshot
 	 * while we are mounting
 	 */
-	mutex_lock(&bdev->bd_fsfreeze_mutex);
+	mutex_lock(&bdev->bd_fsfreeze_blktrace_mutex);
 	if (bdev->bd_fsfreeze_count > 0) {
-		mutex_unlock(&bdev->bd_fsfreeze_mutex);
+		mutex_unlock(&bdev->bd_fsfreeze_blktrace_mutex);
 		error = -EBUSY;
 		goto error_bdev;
 	}
 	s = sget(fs_type, test_bdev_super, set_bdev_super, flags | SB_NOSEC,
 		 bdev);
-	mutex_unlock(&bdev->bd_fsfreeze_mutex);
+	mutex_unlock(&bdev->bd_fsfreeze_blktrace_mutex);
 	if (IS_ERR(s))
 		goto error_s;
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 330b572..16c4297 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -448,8 +448,9 @@ struct block_device {
 
 	/* The counter of freeze processes */
 	int			bd_fsfreeze_count;
-	/* Mutex for freeze and blktrace */
-	struct mutex		bd_fsfreeze_mutex;
+
+	/* Mutex for both freeze and blktrace */
+	struct mutex		bd_fsfreeze_blktrace_mutex;
 } __randomize_layout;
 
 /*
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index 7cd5d1d..463a470 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -656,7 +656,7 @@ int blk_trace_startstop(struct request_queue *q, int start)
  * Protection from multiple readers and writers accessing blktrace data
  * concurrently is still required. The bd_mutex was used for this purpose.
  * That could lead to deadlock with concurrent block device deletion and
- * sysfs access. Instead, the block device bd_fsfreeze_mutex is now
+ * sysfs access. Instead, the block device bd_fsfreeze_blktrace_mutex is now
  * overloaded for blktrace data protection. Like freeze/thaw, blktrace
  * functionality is not frequently used. There is no point in adding
  * one more mutex to the block_device structure just for blktrace.
@@ -679,7 +679,7 @@ int blk_trace_ioctl(struct block_device *bdev, unsigned cmd, char __user *arg)
 	if (!q)
 		return -ENXIO;
 
-	mutex_lock(&bdev->bd_fsfreeze_mutex);
+	mutex_lock(&bdev->bd_fsfreeze_blktrace_mutex);
 
 	switch (cmd) {
 	case BLKTRACESETUP:
@@ -705,7 +705,7 @@ int blk_trace_ioctl(struct block_device *bdev, unsigned cmd, char __user *arg)
 		break;
 	}
 
-	mutex_unlock(&bdev->bd_fsfreeze_mutex);
+	mutex_unlock(&bdev->bd_fsfreeze_blktrace_mutex);
 	return ret;
 }
 
@@ -1741,7 +1741,7 @@ static ssize_t sysfs_blk_trace_attr_show(struct device *dev,
 	if (q == NULL)
 		goto out_bdput;
 
-	mutex_lock(&bdev->bd_fsfreeze_mutex);
+	mutex_lock(&bdev->bd_fsfreeze_blktrace_mutex);
 
 	if (attr == &dev_attr_enable) {
 		ret = sprintf(buf, "%u\n", !!q->blk_trace);
@@ -1760,7 +1760,7 @@ static ssize_t sysfs_blk_trace_attr_show(struct device *dev,
 		ret = sprintf(buf, "%llu\n", q->blk_trace->end_lba);
 
 out_unlock_bdev:
-	mutex_unlock(&bdev->bd_fsfreeze_mutex);
+	mutex_unlock(&bdev->bd_fsfreeze_blktrace_mutex);
 out_bdput:
 	bdput(bdev);
 out:
@@ -1802,7 +1802,7 @@ static ssize_t sysfs_blk_trace_attr_store(struct device *dev,
 	if (q == NULL)
 		goto out_bdput;
 
-	mutex_lock(&bdev->bd_fsfreeze_mutex);
+	mutex_lock(&bdev->bd_fsfreeze_blktrace_mutex);
 
 	if (attr == &dev_attr_enable) {
 		if (value)
@@ -1828,7 +1828,7 @@ static ssize_t sysfs_blk_trace_attr_store(struct device *dev,
 	}
 
 out_unlock_bdev:
-	mutex_unlock(&bdev->bd_fsfreeze_mutex);
+	mutex_unlock(&bdev->bd_fsfreeze_blktrace_mutex);
 out_bdput:
 	bdput(bdev);
 out:
-- 
1.8.3.1

Re: [PATCH v6 0/2] blktrace: Fix deadlock problem

[Steven Rostedt]

Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

for the series.

Jens, feel free to take this in your tree.

-- Steve


On Mon, 18 Sep 2017 14:53:49 -0400
Waiman Long <longman@redhat.com> wrote:

>  v6:
>   - Add a second patch to rename the bd_fsfreeze_mutex to
>     bd_fsfreeze_blktrace_mutex.
> 
>  v5:
>   - Overload the bd_fsfreeze_mutex in block_device structure for
>     blktrace protection.
> 
>  v4:
>   - Use blktrace_mutex in blk_trace_ioctl() as well.
> 
>  v3:
>   - Use a global blktrace_mutex to serialize sysfs attribute accesses
>     instead of the bd_mutex.
> 
>  v2:
>   - Use READ_ONCE() and smp_store_mb() to read and write bd_deleting.
>   - Check for signal in the mutex_trylock loops.
>   - Use usleep() instead of schedule() for RT tasks.
> 
> This patchset fixes a potential blktrace deadlock problem between
> block device deletion and sysfs operations.
> 
> Waiman Long (2):
>   blktrace: Fix potentail deadlock between delete & sysfs ops
>   block_dev: Rename bd_fsfreeze_mutex
> 
>  fs/block_dev.c          | 14 +++++++-------
>  fs/gfs2/ops_fstype.c    |  6 +++---
>  fs/nilfs2/super.c       |  6 +++---
>  fs/super.c              |  6 +++---
>  include/linux/fs.h      |  5 +++--
>  kernel/trace/blktrace.c | 26 ++++++++++++++++++++------
>  6 files changed, 39 insertions(+), 24 deletions(-)
> 

[Cluster-devel] [PATCH v6 0/2] blktrace: Fix deadlock problem

[Steven Rostedt]

Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

for the series.

Jens, feel free to take this in your tree.

-- Steve


On Mon, 18 Sep 2017 14:53:49 -0400
Waiman Long <longman@redhat.com> wrote:

>  v6:
>   - Add a second patch to rename the bd_fsfreeze_mutex to
>     bd_fsfreeze_blktrace_mutex.
> 
>  v5:
>   - Overload the bd_fsfreeze_mutex in block_device structure for
>     blktrace protection.
> 
>  v4:
>   - Use blktrace_mutex in blk_trace_ioctl() as well.
> 
>  v3:
>   - Use a global blktrace_mutex to serialize sysfs attribute accesses
>     instead of the bd_mutex.
> 
>  v2:
>   - Use READ_ONCE() and smp_store_mb() to read and write bd_deleting.
>   - Check for signal in the mutex_trylock loops.
>   - Use usleep() instead of schedule() for RT tasks.
> 
> This patchset fixes a potential blktrace deadlock problem between
> block device deletion and sysfs operations.
> 
> Waiman Long (2):
>   blktrace: Fix potentail deadlock between delete & sysfs ops
>   block_dev: Rename bd_fsfreeze_mutex
> 
>  fs/block_dev.c          | 14 +++++++-------
>  fs/gfs2/ops_fstype.c    |  6 +++---
>  fs/nilfs2/super.c       |  6 +++---
>  fs/super.c              |  6 +++---
>  include/linux/fs.h      |  5 +++--
>  kernel/trace/blktrace.c | 26 ++++++++++++++++++++------
>  6 files changed, 39 insertions(+), 24 deletions(-)
> 

Re: [PATCH v6 2/2] block_dev: Rename bd_fsfreeze_mutex

[Christoph Hellwig]

Don't rename it to a way to long name.  Either add a separate mutex
for your purpose (unless there is interaction between freezing and
blktrace, which I doubt), or properly comment the usage.

[Cluster-devel] [PATCH v6 2/2] block_dev: Rename bd_fsfreeze_mutex

[Christoph Hellwig]

Don't rename it to a way to long name.  Either add a separate mutex
for your purpose (unless there is interaction between freezing and
blktrace, which I doubt), or properly comment the usage.

Re: [PATCH v6 2/2] block_dev: Rename bd_fsfreeze_mutex

[Christoph Hellwig]

Don't rename it to a way to long name.  Either add a separate mutex
for your purpose (unless there is interaction between freezing and
blktrace, which I doubt), or properly comment the usage.
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 1/2] blktrace: Fix potentail deadlock between delete & sysfs ops

[Christoph Hellwig]

Taking a look at this it seems like using a lock in struct block_device
isn't the right thing to do anyway - all the action is on fields in
struct blk_trace, so having a lock inside that would make a lot more
sense.

It would also help to document what exactly we're actually protecting.

[Cluster-devel] [PATCH v6 1/2] blktrace: Fix potentail deadlock between delete & sysfs ops

[Christoph Hellwig]

Taking a look at this it seems like using a lock in struct block_device
isn't the right thing to do anyway - all the action is on fields in
struct blk_trace, so having a lock inside that would make a lot more
sense.

It would also help to document what exactly we're actually protecting.

Re: [PATCH v6 1/2] blktrace: Fix potentail deadlock between delete & sysfs ops

[Christoph Hellwig]

Taking a look at this it seems like using a lock in struct block_device
isn't the right thing to do anyway - all the action is on fields in
struct blk_trace, so having a lock inside that would make a lot more
sense.

It would also help to document what exactly we're actually protecting.
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 2/2] block_dev: Rename bd_fsfreeze_mutex

[Waiman Long]

On 09/18/2017 07:47 PM, Christoph Hellwig wrote:
> Don't rename it to a way to long name.  Either add a separate mutex
> for your purpose (unless there is interaction between freezing and
> blktrace, which I doubt), or properly comment the usage.

I would agree with you if the long name causes the expressions hard to
read. In this particular case, it is just the single parameter to the
mutex_lock() and mutex_unlock() functions. There is no confusion and
overly long lines. So I think it is OK. In fact, I got the opposite
advices in the past that some people prefer long descriptive names than
short and cryptic names.

Cheers,
Longman

[Cluster-devel] [PATCH v6 2/2] block_dev: Rename bd_fsfreeze_mutex

[Waiman Long]

On 09/18/2017 07:47 PM, Christoph Hellwig wrote:
> Don't rename it to a way to long name.  Either add a separate mutex
> for your purpose (unless there is interaction between freezing and
> blktrace, which I doubt), or properly comment the usage.

I would agree with you if the long name causes the expressions hard to
read. In this particular case, it is just the single parameter to the
mutex_lock() and mutex_unlock() functions. There is no confusion and
overly long lines. So I think it is OK. In fact, I got the opposite
advices in the past that some people prefer long descriptive names than
short and cryptic names.

Cheers,
Longman

Re: [PATCH v6 1/2] blktrace: Fix potentail deadlock between delete & sysfs ops

[Waiman Long]

On 09/18/2017 08:01 PM, Christoph Hellwig wrote:
> Taking a look at this it seems like using a lock in struct block_device
> isn't the right thing to do anyway - all the action is on fields in
> struct blk_trace, so having a lock inside that would make a lot more
> sense.
>
> It would also help to document what exactly we're actually protecting.

I think I documented in the patch that the lock has to protect changes
in the blktrace structure as well as the allocation and destruction of
it. Because of that, it can't be put inside the blktrace structure. The
original code use the bd_mutex of the block_device structure. I just
change the code to use another bd_fsfreeze_mutex in the same structure.

In an earlier patch version, I used a global blktrace mutex. This was
deemed to be not scalable enough and so I now use the bd_fsfreeze_mutex
instead.

Cheers,
Longman

[Cluster-devel] [PATCH v6 1/2] blktrace: Fix potentail deadlock between delete & sysfs ops

[Waiman Long]

On 09/18/2017 08:01 PM, Christoph Hellwig wrote:
> Taking a look at this it seems like using a lock in struct block_device
> isn't the right thing to do anyway - all the action is on fields in
> struct blk_trace, so having a lock inside that would make a lot more
> sense.
>
> It would also help to document what exactly we're actually protecting.

I think I documented in the patch that the lock has to protect changes
in the blktrace structure as well as the allocation and destruction of
it. Because of that, it can't be put inside the blktrace structure. The
original code use the bd_mutex of the block_device structure. I just
change the code to use another bd_fsfreeze_mutex in the same structure.

In an earlier patch version, I used a global blktrace mutex. This was
deemed to be not scalable enough and so I now use the bd_fsfreeze_mutex
instead.

Cheers,
Longman

Re: [PATCH v6 1/2] blktrace: Fix potentail deadlock between delete & sysfs ops

[Christoph Hellwig]

On Tue, Sep 19, 2017 at 08:49:12AM -0400, Waiman Long wrote:
> On 09/18/2017 08:01 PM, Christoph Hellwig wrote:
> > Taking a look at this it seems like using a lock in struct block_device
> > isn't the right thing to do anyway - all the action is on fields in
> > struct blk_trace, so having a lock inside that would make a lot more
> > sense.
> >
> > It would also help to document what exactly we're actually protecting.
> 
> I think I documented in the patch that the lock has to protect changes
> in the blktrace structure as well as the allocation and destruction of
> it. Because of that, it can't be put inside the blktrace structure. The
> original code use the bd_mutex of the block_device structure. I just
> change the code to use another bd_fsfreeze_mutex in the same structure.

Either way it has absolutely nothing to do with struct block_device,
given that struct blk_trace hangs off the request_queue.

Reusing a mutex just because it happens to live in a structure also
generally is a bad idea if the protected data is in no way related.

[Cluster-devel] [PATCH v6 1/2] blktrace: Fix potentail deadlock between delete & sysfs ops

[Christoph Hellwig]

On Tue, Sep 19, 2017 at 08:49:12AM -0400, Waiman Long wrote:
> On 09/18/2017 08:01 PM, Christoph Hellwig wrote:
> > Taking a look at this it seems like using a lock in struct block_device
> > isn't the right thing to do anyway - all the action is on fields in
> > struct blk_trace, so having a lock inside that would make a lot more
> > sense.
> >
> > It would also help to document what exactly we're actually protecting.
> 
> I think I documented in the patch that the lock has to protect changes
> in the blktrace structure as well as the allocation and destruction of
> it. Because of that, it can't be put inside the blktrace structure. The
> original code use the bd_mutex of the block_device structure. I just
> change the code to use another bd_fsfreeze_mutex in the same structure.

Either way it has absolutely nothing to do with struct block_device,
given that struct blk_trace hangs off the request_queue.

Reusing a mutex just because it happens to live in a structure also
generally is a bad idea if the protected data is in no way related.

Re: [PATCH v6 1/2] blktrace: Fix potentail deadlock between delete & sysfs ops

[Waiman Long]

On 09/19/2017 10:38 AM, Christoph Hellwig wrote:
> On Tue, Sep 19, 2017 at 08:49:12AM -0400, Waiman Long wrote:
>> On 09/18/2017 08:01 PM, Christoph Hellwig wrote:
>>> Taking a look at this it seems like using a lock in struct block_devi=
ce
>>> isn't the right thing to do anyway - all the action is on fields in
>>> struct blk_trace, so having a lock inside that would make a lot more
>>> sense.
>>>
>>> It would also help to document what exactly we're actually protecting=
=2E
>> I think I documented in the patch that the lock has to protect changes=

>> in the blktrace structure as well as the allocation and destruction of=

>> it. Because of that, it can't be put inside the blktrace structure. Th=
e
>> original code use the bd_mutex of the block_device structure. I just
>> change the code to use another bd_fsfreeze_mutex in the same structure=
=2E
> Either way it has absolutely nothing to do with struct block_device,
> given that struct blk_trace hangs off the request_queue.
>
> Reusing a mutex just because it happens to live in a structure also
> generally is a bad idea if the protected data is in no way related.

I was trying not to add a new mutex to a structure just for blktrace as
it is an optional feature that is enabled only if the
CONFIG_BLK_DEV_IO_TRACE config option is defined and it will only need
to be taken occasionally.

As filesystem freeze looks orthogonal to blktrace and the mutex also
looks to be used sparingly, I think it is a good match to overload it to
control blktrace as well.

I could modify the patch to use a mutex in the request_queue structure.
The current sysfs_lock mutex has about 74 references. So I am not
totally sure if it is safe to reuse. So the only option is to add
something like

#ifdef CONFIG_BLK_DEV_IO_TRACE
struct mutex blktrace_mutex;
#endif

to the request_queue structure. That structure is large enough that
adding a mutex won't increase the size much percentage-wise.

I would like to keep the current patch as is as I don't see any problem
with it. However, I can revise the patch as discussed above if you guys
prefer that alternative.

Cheers,
Longman

Re: [PATCH v6 1/2] blktrace: Fix potentail deadlock between delete & sysfs ops

[Waiman Long]

On 09/19/2017 10:38 AM, Christoph Hellwig wrote:
> On Tue, Sep 19, 2017 at 08:49:12AM -0400, Waiman Long wrote:
>> On 09/18/2017 08:01 PM, Christoph Hellwig wrote:
>>> Taking a look at this it seems like using a lock in struct block_device
>>> isn't the right thing to do anyway - all the action is on fields in
>>> struct blk_trace, so having a lock inside that would make a lot more
>>> sense.
>>>
>>> It would also help to document what exactly we're actually protecting.
>> I think I documented in the patch that the lock has to protect changes
>> in the blktrace structure as well as the allocation and destruction of
>> it. Because of that, it can't be put inside the blktrace structure. The
>> original code use the bd_mutex of the block_device structure. I just
>> change the code to use another bd_fsfreeze_mutex in the same structure.
> Either way it has absolutely nothing to do with struct block_device,
> given that struct blk_trace hangs off the request_queue.
>
> Reusing a mutex just because it happens to live in a structure also
> generally is a bad idea if the protected data is in no way related.

I was trying not to add a new mutex to a structure just for blktrace as
it is an optional feature that is enabled only if the
CONFIG_BLK_DEV_IO_TRACE config option is defined and it will only need
to be taken occasionally.

As filesystem freeze looks orthogonal to blktrace and the mutex also
looks to be used sparingly, I think it is a good match to overload it to
control blktrace as well.

I could modify the patch to use a mutex in the request_queue structure.
The current sysfs_lock mutex has about 74 references. So I am not
totally sure if it is safe to reuse. So the only option is to add
something like

#ifdef CONFIG_BLK_DEV_IO_TRACE
struct mutex blktrace_mutex;
#endif

to the request_queue structure. That structure is large enough that
adding a mutex won't increase the size much percentage-wise.

I would like to keep the current patch as is as I don't see any problem
with it. However, I can revise the patch as discussed above if you guys
prefer that alternative.

Cheers,
Longman

[Cluster-devel] [PATCH v6 1/2] blktrace: Fix potentail deadlock between delete & sysfs ops

[Waiman Long]

On 09/19/2017 10:38 AM, Christoph Hellwig wrote:
> On Tue, Sep 19, 2017 at 08:49:12AM -0400, Waiman Long wrote:
>> On 09/18/2017 08:01 PM, Christoph Hellwig wrote:
>>> Taking a look at this it seems like using a lock in struct block_device
>>> isn't the right thing to do anyway - all the action is on fields in
>>> struct blk_trace, so having a lock inside that would make a lot more
>>> sense.
>>>
>>> It would also help to document what exactly we're actually protecting.
>> I think I documented in the patch that the lock has to protect changes
>> in the blktrace structure as well as the allocation and destruction of
>> it. Because of that, it can't be put inside the blktrace structure. The
>> original code use the bd_mutex of the block_device structure. I just
>> change the code to use another bd_fsfreeze_mutex in the same structure.
> Either way it has absolutely nothing to do with struct block_device,
> given that struct blk_trace hangs off the request_queue.
>
> Reusing a mutex just because it happens to live in a structure also
> generally is a bad idea if the protected data is in no way related.

I was trying not to add a new mutex to a structure just for blktrace as
it is an optional feature that is enabled only if the
CONFIG_BLK_DEV_IO_TRACE config option is defined and it will only need
to be taken occasionally.

As filesystem freeze looks orthogonal to blktrace and the mutex also
looks to be used sparingly, I think it is a good match to overload it to
control blktrace as well.

I could modify the patch to use a mutex in the request_queue structure.
The current sysfs_lock mutex has about 74 references. So I am not
totally sure if it is safe to reuse. So the only option is to add
something like

#ifdef CONFIG_BLK_DEV_IO_TRACE
struct mutex blktrace_mutex;
#endif

to the request_queue structure. That structure is large enough that
adding a mutex won't increase the size much percentage-wise.

I would like to keep the current patch as is as I don't see any problem
with it. However, I can revise the patch as discussed above if you guys
prefer that alternative.

Cheers,
Longman

Re: [PATCH v6 1/2] blktrace: Fix potentail deadlock between delete & sysfs ops

[Christoph Hellwig]

On Tue, Sep 19, 2017 at 11:58:34AM -0400, Waiman Long wrote:
> I was trying not to add a new mutex to a structure just for blktrace as
> it is an optional feature that is enabled only if the
> CONFIG_BLK_DEV_IO_TRACE config option is defined and it will only need
> to be taken occasionally.

So?  Make the lock optional, too.

> As filesystem freeze looks orthogonal to blktrace and the mutex also
> looks to be used sparingly, I think it is a good match to overload it to
> control blktrace as well.

If the functionally is orthogonal that is a reason not to share a lock,
not to the contrary.

> I could modify the patch to use a mutex in the request_queue structure.
> The current sysfs_lock mutex has about 74 references. So I am not
> totally sure if it is safe to reuse. So the only option is to add
> something like

> 
> #ifdef CONFIG_BLK_DEV_IO_TRACE
> struct mutex blktrace_mutex;
> #endif
> 
> to the request_queue structure. That structure is large enough that
> adding a mutex won't increase the size much percentage-wise.

Call it blk_trace mutex and move it right next to the blk_trace
structure:

ifdef CONFIG_BLK_DEV_IO_TRACE
        struct blk_trace        *blk_trace;
	struct mutex		blk_trace_mutex;
#endif

which makes it completely obvious to any read what you are protecting
with it.

[Cluster-devel] [PATCH v6 1/2] blktrace: Fix potentail deadlock between delete & sysfs ops

[Christoph Hellwig]

On Tue, Sep 19, 2017 at 11:58:34AM -0400, Waiman Long wrote:
> I was trying not to add a new mutex to a structure just for blktrace as
> it is an optional feature that is enabled only if the
> CONFIG_BLK_DEV_IO_TRACE config option is defined and it will only need
> to be taken occasionally.

So?  Make the lock optional, too.

> As filesystem freeze looks orthogonal to blktrace and the mutex also
> looks to be used sparingly, I think it is a good match to overload it to
> control blktrace as well.

If the functionally is orthogonal that is a reason not to share a lock,
not to the contrary.

> I could modify the patch to use a mutex in the request_queue structure.
> The current sysfs_lock mutex has about 74 references. So I am not
> totally sure if it is safe to reuse. So the only option is to add
> something like

> 
> #ifdef CONFIG_BLK_DEV_IO_TRACE
> struct mutex blktrace_mutex;
> #endif
> 
> to the request_queue structure. That structure is large enough that
> adding a mutex won't increase the size much percentage-wise.

Call it blk_trace mutex and move it right next to the blk_trace
structure:

ifdef CONFIG_BLK_DEV_IO_TRACE
        struct blk_trace        *blk_trace;
	struct mutex		blk_trace_mutex;
#endif

which makes it completely obvious to any read what you are protecting
with it.

Re: [PATCH v6 1/2] blktrace: Fix potentail deadlock between delete & sysfs ops

[Steven Rostedt]

On Tue, 19 Sep 2017 13:41:35 -0700
Christoph Hellwig <hch@infradead.org> wrote:

> Call it blk_trace mutex and move it right next to the blk_trace
> structure:
> 
> ifdef CONFIG_BLK_DEV_IO_TRACE
>         struct blk_trace        *blk_trace;
> 	struct mutex		blk_trace_mutex;
> #endif
> 
> which makes it completely obvious to any read what you are protecting
> with it.

As a 1000ft away bystander, this appears to be the most logical
solution.

-- Steve

[Cluster-devel] [PATCH v6 1/2] blktrace: Fix potentail deadlock between delete & sysfs ops

[Steven Rostedt]

On Tue, 19 Sep 2017 13:41:35 -0700
Christoph Hellwig <hch@infradead.org> wrote:

> Call it blk_trace mutex and move it right next to the blk_trace
> structure:
> 
> ifdef CONFIG_BLK_DEV_IO_TRACE
>         struct blk_trace        *blk_trace;
> 	struct mutex		blk_trace_mutex;
> #endif
> 
> which makes it completely obvious to any read what you are protecting
> with it.

As a 1000ft away bystander, this appears to be the most logical
solution.

-- Steve