Patchwork [1/3] block: Create sysfs knobs to override FLUSH/FUA support flags

login
register
mail settings
Submitter Darrick J. Wong
Date Jan. 26, 2011, 7:16 a.m.
Message ID <20110126071626.GI27190@tux1.beaverton.ibm.com>
Download mbox | patch
Permalink /patch/80453/
State New
Headers show

Comments

Darrick J. Wong - Jan. 26, 2011, 7:16 a.m.
This patch is the first in a series to refactor the barrier= mount options out
of the filesystem code.  This patch adds sysfs knobs to disable flush and FUA;
of course, the automatic default is the safe choice, i.e. to leave them
enabled.  Obviously, only a seasoned administrator should ever be overriding
the defaults.

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
---

 block/blk-settings.c   |    1 +
 block/blk-sysfs.c      |   66 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/blkdev.h |    1 +
 3 files changed, 68 insertions(+), 0 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo - Jan. 26, 2011, 9:30 a.m.
On Tue, Jan 25, 2011 at 11:16:26PM -0800, Darrick J. Wong wrote:
> This patch is the first in a series to refactor the barrier= mount options out
> of the filesystem code.  This patch adds sysfs knobs to disable flush and FUA;
> of course, the automatic default is the safe choice, i.e. to leave them
> enabled.  Obviously, only a seasoned administrator should ever be overriding
> the defaults.

Hmmm... wouldn't it be better to just export flush and fua instead of
ignore_*?  So that the admin can turn things on and off as [s]he seems
fit?  Also, it might be better to export them in a single attribute,
say cache_control or something.  Only subset of the combinations make
sense anyway - none, flush, flush_fua.

Thanks.
Darrick J. Wong - Jan. 26, 2011, 5 p.m.
On Wed, Jan 26, 2011 at 10:30:51AM +0100, Tejun Heo wrote:
> On Tue, Jan 25, 2011 at 11:16:26PM -0800, Darrick J. Wong wrote:
> > This patch is the first in a series to refactor the barrier= mount options out
> > of the filesystem code.  This patch adds sysfs knobs to disable flush and FUA;
> > of course, the automatic default is the safe choice, i.e. to leave them
> > enabled.  Obviously, only a seasoned administrator should ever be overriding
> > the defaults.
> 
> Hmmm... wouldn't it be better to just export flush and fua instead of
> ignore_*?  So that the admin can turn things on and off as [s]he seems

I considered having a general knob to override the automatic FLUSH/FUA
detection, but I thought that it wasn't a good idea to provide a mechanism to
enable features that devices don't advertise.  Mostly I was imagining horror
scenarios like USB storage devices that claim no write cache and but then catch
on fire if someone sends flush anyway.  Not using advertised features seemed
less risky.

> fit?  Also, it might be better to export them in a single attribute,
> say cache_control or something.  Only subset of the combinations make
> sense anyway - none, flush, flush_fua.

I agree.  It could be simplified even further to a simple boolean that means
"use neither" or "use whatever's supported".

--D
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greg KH - Feb. 5, 2011, 4:20 p.m.
On Tue, Jan 25, 2011 at 11:16:26PM -0800, Darrick J. Wong wrote:
> This patch is the first in a series to refactor the barrier= mount options out
> of the filesystem code.  This patch adds sysfs knobs to disable flush and FUA;

If you add sysfs files, you must also add documentation for these files
in Documentation/ABI/.

Please do that in this patch series.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/block/blk-settings.c b/block/blk-settings.c
index 36c8c1f..719c990 100644
--- a/block/blk-settings.c
+++ b/block/blk-settings.c
@@ -802,6 +802,7 @@  void blk_queue_flush(struct request_queue *q, unsigned int flush)
 		flush &= ~REQ_FUA;
 
 	q->flush_flags = flush & (REQ_FLUSH | REQ_FUA);
+	q->hw_flush_flags = q->flush_flags;
 }
 EXPORT_SYMBOL_GPL(blk_queue_flush);
 
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index 41fb691..af872a8 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -31,6 +31,58 @@  queue_var_store(unsigned long *var, const char *page, size_t count)
 	return count;
 }
 
+static ssize_t queue_flushflag_ignore_show(struct request_queue *q, char *page,
+					   unsigned int which)
+{
+	if (!(q->hw_flush_flags & which))
+		return sprintf(page, "0\n");
+
+	if (!(q->flush_flags & which))
+		return sprintf(page, "1\n");
+
+	return sprintf(page, "0\n");
+}
+
+static ssize_t
+queue_flushflag_ignore_store(struct request_queue *q, const char *page,
+			     size_t count, unsigned int which)
+{
+	unsigned long ignore;
+	ssize_t ret = queue_var_store(&ignore, page, count);
+
+	if (!(q->hw_flush_flags & which))
+		return -EINVAL;
+
+	if (ignore)
+		q->flush_flags &= ~which;
+	else
+		q->flush_flags |= which;
+
+	return ret;
+}
+
+static ssize_t queue_ignore_flush_show(struct request_queue *q, char *page)
+{
+	return queue_flushflag_ignore_show(q, page, REQ_FLUSH);
+}
+
+static ssize_t queue_ignore_flush_store(struct request_queue *q,
+					const char *page, size_t count)
+{
+	return queue_flushflag_ignore_store(q, page, count, REQ_FLUSH);
+}
+
+static ssize_t queue_ignore_fua_show(struct request_queue *q, char *page)
+{
+	return queue_flushflag_ignore_show(q, page, REQ_FUA);
+}
+
+static ssize_t queue_ignore_fua_store(struct request_queue *q,
+				      const char *page, size_t count)
+{
+	return queue_flushflag_ignore_store(q, page, count, REQ_FUA);
+}
+
 static ssize_t queue_requests_show(struct request_queue *q, char *page)
 {
 	return queue_var_show(q->nr_requests, (page));
@@ -265,6 +317,18 @@  queue_rq_affinity_store(struct request_queue *q, const char *page, size_t count)
 	return ret;
 }
 
+static struct queue_sysfs_entry queue_ignore_flush_entry = {
+	.attr = {.name = "ignore_flush", .mode = S_IRUGO | S_IWUSR },
+	.show = queue_ignore_flush_show,
+	.store = queue_ignore_flush_store,
+};
+
+static struct queue_sysfs_entry queue_ignore_fua_entry = {
+	.attr = {.name = "ignore_fua", .mode = S_IRUGO | S_IWUSR },
+	.show = queue_ignore_fua_show,
+	.store = queue_ignore_fua_store,
+};
+
 static struct queue_sysfs_entry queue_requests_entry = {
 	.attr = {.name = "nr_requests", .mode = S_IRUGO | S_IWUSR },
 	.show = queue_requests_show,
@@ -380,6 +444,8 @@  static struct queue_sysfs_entry queue_random_entry = {
 };
 
 static struct attribute *default_attrs[] = {
+	&queue_ignore_flush_entry.attr,
+	&queue_ignore_fua_entry.attr,
 	&queue_requests_entry.attr,
 	&queue_ra_entry.attr,
 	&queue_max_hw_sectors_entry.attr,
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 8a082a5..daa4e6b 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -368,6 +368,7 @@  struct request_queue
 	 * for flush operations
 	 */
 	unsigned int		flush_flags;
+	unsigned int		hw_flush_flags;
 	unsigned int		flush_pending_idx:1;
 	unsigned int		flush_running_idx:1;
 	unsigned long		flush_pending_since;