[PATCH 0/3] Fix warnings in __mark_inode_dirty

[PATCH 0/3] Fix warnings in __mark_inode_dirty

[Jan Kara]

  Hi,

  patches below fix (some of) warnings in __mark_inode_dirty complaining about
unregistered BDI (https://bugzilla.kernel.org/show_bug.cgi?id=16312).  The
issue is that device inodes describing non-writeable BDI can still get dirty
but they would be filed to writeback lists of these non-writeable BDI's.
Normally, we would just silently throw away the inode modification (bug) but
some unwriteable BDI's (like the one of /dev/zero) aren't marked as such and so
the code in __mark_inode_dirty gets upset that writeback isn't properly setup
although BDI is writeable.
  The solution is slightly hacky but it should be a reasonable short term
solution until we resolve the principial issue that device inode should be
written to a different BDI than inode's data. Christoph wrote he has some
ideas in this direction.

								Honza

PS: There might be another issue in sd.c code triggering this warning --- BDI
is registered from async callback (add_disk() called from sd_probe_async())
which might happen after userspace tries to access the device if I'm right.
At least this is the only explanation I have for some of the warnings.

[PATCH 1/3] bdi: Use parent filesystem BDI for inodes not capable of writeback

[Jan Kara]

If inode->i_mapping->backing_dev_info is not capable of writeback
but the inode gets dirty (because inode itself has changed), use
inode lists of a bdi of a filesystem holding the device inode.
Forking the flusher thread just to do the writeout of a single
inode would be silly and generally bdi needn't be prepared to do
any writeback.

This is part of fixing a bug
https://bugzilla.kernel.org/show_bug.cgi?id=16312.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/fs-writeback.c |   17 +++++++++++++++--
 1 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index d5be169..00865aa 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -28,8 +28,6 @@
 #include <linux/buffer_head.h>
 #include "internal.h"
 
-#define inode_to_bdi(inode)	((inode)->i_mapping->backing_dev_info)
-
 /*
  * We don't actually have pdflush, but this one is exported though /proc...
  */
@@ -50,6 +48,21 @@ struct wb_writeback_work {
 	struct completion *done;	/* set if the caller waits */
 };
 
+static struct backing_dev_info *inode_to_bdi(struct inode *inode)
+{
+	struct backing_dev_info *bdi = inode->i_mapping->backing_dev_info;
+
+	/*
+	 * This is a hack but it solves a problem with device inode
+	 * for e.g. /dev/zero getting dirty (via touch or so) and confusing
+	 * writeback code. In such cases we return the "parent" filesystem's
+	 * bdi.
+	 */
+	if (bdi_cap_writeback_dirty(bdi))
+		return bdi;
+	return inode->i_sb->s_bdi;
+}
+
 /**
  * writeback_in_progress - determine whether there is writeback in progress
  * @bdi: the device's backing_dev_info structure.
-- 
1.6.4.2

Re: [PATCH 1/3] bdi: Use parent filesystem BDI for inodes not capable of writeback

[Christoph Hellwig]

> +static struct backing_dev_info *inode_to_bdi(struct inode *inode)
> +{
> +	struct backing_dev_info *bdi = inode->i_mapping->backing_dev_info;
> +
> +	/*
> +	 * This is a hack but it solves a problem with device inode
> +	 * for e.g. /dev/zero getting dirty (via touch or so) and confusing
> +	 * writeback code. In such cases we return the "parent" filesystem's
> +	 * bdi.
> +	 */
> +	if (bdi_cap_writeback_dirty(bdi))
> +		return bdi;
> +	return inode->i_sb->s_bdi;

When do we ever have a writeback-capable bdi that sits inside another
filesystem?  I think just always using inode->i_sb->s_bdi is the right
thing here.

And btw, having a BDI_CAP_NO_WRITEBACK instead of a BDI_CAP_WRITEBACK
is rather dumb, we'd better fix it up while we're at it.

Re: [PATCH 1/3] bdi: Use parent filesystem BDI for inodes not capable of writeback

[Jan Kara]

On Tue 27-07-10 13:09:03, Christoph Hellwig wrote:
> > +static struct backing_dev_info *inode_to_bdi(struct inode *inode)
> > +{
> > +	struct backing_dev_info *bdi = inode->i_mapping->backing_dev_info;
> > +
> > +	/*
> > +	 * This is a hack but it solves a problem with device inode
> > +	 * for e.g. /dev/zero getting dirty (via touch or so) and confusing
> > +	 * writeback code. In such cases we return the "parent" filesystem's
> > +	 * bdi.
> > +	 */
> > +	if (bdi_cap_writeback_dirty(bdi))
> > +		return bdi;
> > +	return inode->i_sb->s_bdi;
> 
> When do we ever have a writeback-capable bdi that sits inside another
> filesystem?  I think just always using inode->i_sb->s_bdi is the right
> thing here.
  Always returning inode->i_sb->s_bdi wouldn't be a right thing IMHO.
That would file inode for /dev/sda to BDI list of tmpfs mounted on /dev/
which isn't what you want...

> And btw, having a BDI_CAP_NO_WRITEBACK instead of a BDI_CAP_WRITEBACK
> is rather dumb, we'd better fix it up while we're at it.
  Yes, that seems reasonable. I'm just not sure how many places one has
to change to fix this and how to find them... Maybe grepping for BDI_CAP_
will be enough but I'm not sure.
								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 1/3] bdi: Use parent filesystem BDI for inodes not capable of writeback

[Christoph Hellwig]

On Tue, Jul 27, 2010 at 07:24:38PM +0200, Jan Kara wrote:
>   Always returning inode->i_sb->s_bdi wouldn't be a right thing IMHO.
> That would file inode for /dev/sda to BDI list of tmpfs mounted on /dev/
> which isn't what you want...

It shouldn't.  Block device nodes are on the bdev filesystems, and
we twist the file->mapping pointer so that all the low-level read/write
code always deals with the bdev fs inode, and not the device node
filesystem.

Re: [PATCH 1/3] bdi: Use parent filesystem BDI for inodes not capable of writeback

[Jan Kara]

On Tue 27-07-10 13:27:31, Christoph Hellwig wrote:
> On Tue, Jul 27, 2010 at 07:24:38PM +0200, Jan Kara wrote:
> >   Always returning inode->i_sb->s_bdi wouldn't be a right thing IMHO.
> > That would file inode for /dev/sda to BDI list of tmpfs mounted on /dev/
> > which isn't what you want...
> 
> It shouldn't.  Block device nodes are on the bdev filesystems, and
  Ok, so inode->i_sb->s_bdi will actually point to noop_backing_dev_info
as set by set_anon_super(). Or am I completely out?

> we twist the file->mapping pointer so that all the low-level read/write
> code always deals with the bdev fs inode, and not the device node
> filesystem.
  Yes, I know this but I fail to see how this influences where </dev/sda's
inode>->i_sb->s_bdi ends up...

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 1/3] bdi: Use parent filesystem BDI for inodes not capable of writeback

[Christoph Hellwig]

On Tue, Jul 27, 2010 at 08:01:31PM +0200, Jan Kara wrote:
> > It shouldn't.  Block device nodes are on the bdev filesystems, and
>   Ok, so inode->i_sb->s_bdi will actually point to noop_backing_dev_info
> as set by set_anon_super(). Or am I completely out?

I think you're right.  This seems rather bad if it's indeed true. I'll
quickly verify it using Dave's new tracing once I've built a block
tree kernel.

> > we twist the file->mapping pointer so that all the low-level read/write
> > code always deals with the bdev fs inode, and not the device node
> > filesystem.
>
> Yes, I know this but I fail to see how this influences where </dev/sda's
> inode>->i_sb->s_bdi ends up...

Ok, let's say we write to /dev/sda.  For this we call

	blkdev_aio_write -> __generic_file_aio_write

There we get the inode we operate on as

	struct file *file = iocb->ki_filp;
	struct address_space * mapping = file->f_mapping;
	struct inode    *inode = mapping->host;

Now file->f_mapping gets set up in blkdev_open as:

	filp->f_mapping = bdev->bd_inode->i_mapping;

so it does not point to the inode of the device node, but an inode on
the bdev filesystem.

Re: [PATCH 1/3] bdi: Use parent filesystem BDI for inodes not capable of writeback

[Christoph Hellwig]

On Tue, Jul 27, 2010 at 04:12:07PM -0400, Christoph Hellwig wrote:
> On Tue, Jul 27, 2010 at 08:01:31PM +0200, Jan Kara wrote:
> > > It shouldn't.  Block device nodes are on the bdev filesystems, and
> >   Ok, so inode->i_sb->s_bdi will actually point to noop_backing_dev_info
> > as set by set_anon_super(). Or am I completely out?
> 
> I think you're right.  This seems rather bad if it's indeed true. I'll
> quickly verify it using Dave's new tracing once I've built a block
> tree kernel.

Indeed it does.  So using ->s_bdi actually is wrong for the block
device node, given that it does set up the bdevfs inode's 
backing_dev_info to the proper one, but can't actually do it for
the per-sb one.

Re: [PATCH 1/3] bdi: Use parent filesystem BDI for inodes not capable of writeback

[Jan Kara]

On Tue 27-07-10 16:21:29, Christoph Hellwig wrote:
> On Tue, Jul 27, 2010 at 04:12:07PM -0400, Christoph Hellwig wrote:
> > On Tue, Jul 27, 2010 at 08:01:31PM +0200, Jan Kara wrote:
> > > > It shouldn't.  Block device nodes are on the bdev filesystems, and
> > >   Ok, so inode->i_sb->s_bdi will actually point to noop_backing_dev_info
> > > as set by set_anon_super(). Or am I completely out?
> > 
> > I think you're right.  This seems rather bad if it's indeed true. I'll
> > quickly verify it using Dave's new tracing once I've built a block
> > tree kernel.
> 
> Indeed it does.  So using ->s_bdi actually is wrong for the block
> device node, given that it does set up the bdevfs inode's 
> backing_dev_info to the proper one, but can't actually do it for
> the per-sb one.
  Yes. So what I do in inode_to_bdi() is necessary (although I agree it
looks a bit ugly).
								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 1/3] bdi: Use parent filesystem BDI for inodes not capable of writeback

[Christoph Hellwig]

On Tue, Jul 27, 2010 at 10:44:07PM +0200, Jan Kara wrote:
>   Yes. So what I do in inode_to_bdi() is necessary (although I agree it
> looks a bit ugly).

Yes, seem like it.  Damn, so much for my plan of never looking at
mapping->backing_dev_info from writeback code.

Re: [PATCH 1/3] bdi: Use parent filesystem BDI for inodes not capable of writeback

[Christoph Hellwig]

Given that we only need the per-mapping one for the bdev fs what about
doing:

/*
 * Return the writeback-relevant backing device for this inode.
 *
 * For a normal filesystem this must always be the bdi hanging off the
 * superblock, given that we only expect one bdi per filesystems in
 * the per-superblock sync functions.  But the block device special
 * filesystem requires a quick given that it contains the internal
 * I/O inodes for block devices on a single superblock.  This works
 * because the block deevice filesystem inodes are never user visible
 * and we will never do a per-superblock sync on it.
 */
static struct backing_dev_info *inode_to_bdi(struct inode *inode)
{
	if (inode->i_sb == blockdev_superblock)
		return inode->i_mapping->backing_dev_info;
	return inode->i_sb->s_bdi;
}

that also avoids the need for doing the bdi capabilities audit ASAP.

Re: [PATCH 1/3] bdi: Use parent filesystem BDI for inodes not capable of writeback

[Jan Kara]

On Thu 29-07-10 04:12:10, Christoph Hellwig wrote:
> Given that we only need the per-mapping one for the bdev fs what about
> doing:
> 
> /*
>  * Return the writeback-relevant backing device for this inode.
>  *
>  * For a normal filesystem this must always be the bdi hanging off the
>  * superblock, given that we only expect one bdi per filesystems in
>  * the per-superblock sync functions.  But the block device special
>  * filesystem requires a quick given that it contains the internal
>  * I/O inodes for block devices on a single superblock.  This works
>  * because the block deevice filesystem inodes are never user visible
>  * and we will never do a per-superblock sync on it.
>  */
> static struct backing_dev_info *inode_to_bdi(struct inode *inode)
> {
> 	if (inode->i_sb == blockdev_superblock)
> 		return inode->i_mapping->backing_dev_info;
> 	return inode->i_sb->s_bdi;
> }
> 
> that also avoids the need for doing the bdi capabilities audit ASAP.
  Well, but I'm not sure we'll help ourselves with having to do audit this
way. Some other pseudofilesystems can play tricks with backing_dev_info as
well and thus inode->i_mapping->backing_dev_info != inode->i_sb->s_bdi. In
particular MTD seems to do such things. As I'm looking at the code, it
looks like a similar case as blockdev filesystem.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[PATCH 2/3] char: Mark /dev/zero and /dev/kmem as not capable of writeback

[Jan Kara]

These devices don't do any writeback but their device inodes still can get
dirty so mark bdi appropriately so that bdi code does the right thing and files
inodes to lists of bdi carrying the device inodes.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 drivers/char/mem.c |    3 ++-
 fs/char_dev.c      |    4 +++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index f54dab8..16a4443 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -788,10 +788,11 @@ static const struct file_operations zero_fops = {
 /*
  * capabilities for /dev/zero
  * - permits private mappings, "copies" are taken of the source of zeros
+ * - no writeback happens
  */
 static struct backing_dev_info zero_bdi = {
 	.name		= "char/mem",
-	.capabilities	= BDI_CAP_MAP_COPY,
+	.capabilities	= BDI_CAP_MAP_COPY | BDI_CAP_NO_ACCT_AND_WRITEBACK,
 };
 
 static const struct file_operations full_fops = {
diff --git a/fs/char_dev.c b/fs/char_dev.c
index d6db933..be7613e 100644
--- a/fs/char_dev.c
+++ b/fs/char_dev.c
@@ -39,7 +39,9 @@ struct backing_dev_info directly_mappable_cdev_bdi = {
 #endif
 		/* permit direct mmap, for read, write or exec */
 		BDI_CAP_MAP_DIRECT |
-		BDI_CAP_READ_MAP | BDI_CAP_WRITE_MAP | BDI_CAP_EXEC_MAP),
+		BDI_CAP_READ_MAP | BDI_CAP_WRITE_MAP | BDI_CAP_EXEC_MAP |
+		/* no writeback happens */
+		BDI_CAP_NO_ACCT_AND_WRITEBACK),
 };
 
 static struct kobj_map *cdev_map;
-- 
1.6.4.2

[PATCH 3/3] bdi: Initialize noop_backing_dev_info properly

[Jan Kara]

Properly initialize this backing dev info so that writeback code does not
barf when getting to it e.g. via sb->s_bdi.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 mm/backing-dev.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index 123bcef..c0f7157 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -29,6 +29,7 @@ EXPORT_SYMBOL_GPL(default_backing_dev_info);
 
 struct backing_dev_info noop_backing_dev_info = {
 	.name		= "noop",
+	.capabilities	= BDI_CAP_NO_ACCT_AND_WRITEBACK,
 };
 EXPORT_SYMBOL_GPL(noop_backing_dev_info);
 
@@ -254,6 +255,7 @@ static int __init default_bdi_init(void)
 	err = bdi_init(&default_backing_dev_info);
 	if (!err)
 		bdi_register(&default_backing_dev_info, NULL, "default");
+	err = bdi_init(&noop_backing_dev_info);
 
 	return err;
 }
-- 
1.6.4.2

Re: [PATCH 3/3] bdi: Initialize noop_backing_dev_info properly

[Christoph Hellwig]

On Tue, Jul 27, 2010 at 07:05:01PM +0200, Jan Kara wrote:
> Properly initialize this backing dev info so that writeback code does not
> barf when getting to it e.g. via sb->s_bdi.

When do we actually get it in s_bdi?  That should never happen.

Re: [PATCH 3/3] bdi: Initialize noop_backing_dev_info properly

[Jan Kara]

On Tue 27-07-10 13:09:39, Christoph Hellwig wrote:
> On Tue, Jul 27, 2010 at 07:05:01PM +0200, Jan Kara wrote:
> > Properly initialize this backing dev info so that writeback code does not
> > barf when getting to it e.g. via sb->s_bdi.
> 
> When do we actually get it in s_bdi?  That should never happen.
  Well, set_anon_super() sets it. I'm not sure if filesystems using this
are supposed to reset it to something else or not... It's a bit messy and
nowhere is actually written how things should be. One can just guess ;)


								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR