[RFC] vfs/inode: For none-block-based filesystems default to sb->s_bdi

[RFC] vfs/inode: For none-block-based filesystems default to sb->s_bdi

[Boaz Harrosh]

In alloc_inode (inode_init_always) in the case that sb->s_bdev
is NULL. Should we not use sb->s_bdi as the default
mapping->backing_dev_info?

This fixes my none-block-based filesystem recent WARN_ON
at fs/fs-writeback.c:87 inode_to_bdi()

If not done here I'll need to do this in 5 different cases
in FS code. (OK the code could enjoy some re-factoring).

It does look logical the question is how many FSs will now
get broken?

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 fs/inode.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index 8646433..200314f 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -181,6 +181,8 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
 
 		bdi = sb->s_bdev->bd_inode->i_mapping->backing_dev_info;
 		mapping->backing_dev_info = bdi;
+	} else {
+		mapping->backing_dev_info = sb->s_bdi;
 	}
 	inode->i_private = NULL;
 	inode->i_mapping = mapping;
-- 
1.7.2

Re: [RFC] vfs/inode: For none-block-based filesystems default to sb->s_bdi

[Boaz Harrosh]

On 10/04/2010 05:46 PM, Boaz Harrosh wrote:
> 
> In alloc_inode (inode_init_always) in the case that sb->s_bdev
> is NULL. Should we not use sb->s_bdi as the default
> mapping->backing_dev_info?
> 
> This fixes my none-block-based filesystem recent WARN_ON
> at fs/fs-writeback.c:87 inode_to_bdi()
> 
> If not done here I'll need to do this in 5 different cases
> in FS code. (OK the code could enjoy some re-factoring).
> 
> It does look logical the question is how many FSs will now
> get broken?
> 
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> ---
>  fs/inode.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 8646433..200314f 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -181,6 +181,8 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
>  
>  		bdi = sb->s_bdev->bd_inode->i_mapping->backing_dev_info;
>  		mapping->backing_dev_info = bdi;
> +	} else {
> +		mapping->backing_dev_info = sb->s_bdi;
>  	}
>  	inode->i_private = NULL;
>  	inode->i_mapping = mapping;

Sorry I've just seen Jan's patch:
From: Jan Kara <jack@suse.cz>
Date: Mon, 27 Sep 2010 23:56:48 +0200
Subject: [PATCH] bdi: Initialize inode->i_mapping.backing_dev_info to sb->s_bdi

Currently, we initialize inode->i_mapping.backing_dev_info to the bdi of device
sb->s_bdev points to. However there is quite a big number of filesystems that
do not set sb->s_bdev (because they do not have one) but do set sb->s_bdi.
These filesystems would generally benefit from setting
inode->i_mapping.backing_dev_info to their s_bdi because otherwise their inodes
would point to default_backing_dev_info and thus dirty inode tracking would
happen there. So change inode initialization code to use sb->s_bdi if it
is available.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/inode.c |   22 ++++++++++++++--------
 1 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index 8646433..e415be4 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -172,15 +172,21 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
 	mapping->writeback_index = 0;
 
 	/*
-	 * If the block_device provides a backing_dev_info for client
-	 * inodes then use that.  Otherwise the inode share the bdev's
-	 * backing_dev_info.
+	 * If the filesystem provides a backing_dev_info for client inodes
+	 * then use that. Otherwise inodes share default_backing_dev_info.
 	 */
-	if (sb->s_bdev) {
-		struct backing_dev_info *bdi;
-
-		bdi = sb->s_bdev->bd_inode->i_mapping->backing_dev_info;
-		mapping->backing_dev_info = bdi;
+	if (sb->s_bdi && sb->s_bdi != &noop_backing_dev_info) {
+		/*
+		 * Catch cases where filesystem might be bitten by using s_bdi
+		 * instead of sb->s_bdev. Can be removed in 2.6.38.
+		 */
+		if (sb->s_bdev) {
+			struct backing_dev_info *bdi =
+			  sb->s_bdev->bd_inode->i_mapping->backing_dev_info;
+			WARN(bdi != sb->s_bdi, "s_bdev bdi %s != s_bdi %s\n",
+			     bdi->name, sb->s_bdi->name);
+		}
+		mapping->backing_dev_info = sb->s_bdi;
 	}
 	inode->i_private = NULL;
 	inode->i_mapping = mapping;

That works for me as well. Was it decided how to solve this? Other wise
I'll need to patch exofs, ASAP for this -rc

Thanks
Boaz
 

Re: [RFC] vfs/inode: For none-block-based filesystems default to sb->s_bdi

[Jan Kara]

  Hi Boaz,

On Mon 04-10-10 18:02:13, Boaz Harrosh wrote:
> Sorry I've just seen Jan's patch:
> From: Jan Kara <jack@suse.cz>
> Date: Mon, 27 Sep 2010 23:56:48 +0200
> Subject: [PATCH] bdi: Initialize inode->i_mapping.backing_dev_info to sb->s_bdi
...
> That works for me as well. Was it decided how to solve this? Other wise
> I'll need to patch exofs, ASAP for this -rc
  In the end, we'll use Christoph's patch
(http://lkml.org/lkml/2010/9/29/76) changing inode_to_bdi() to be
more conservative and also the warning will be gone. So you don't have to
patch anything...

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

Re: [RFC] vfs/inode: For none-block-based filesystems default to sb->s_bdi

[Boaz Harrosh]

On 10/05/2010 04:32 AM, Jan Kara wrote:
>   Hi Boaz,
> 
> On Mon 04-10-10 18:02:13, Boaz Harrosh wrote:
>> Sorry I've just seen Jan's patch:
>> From: Jan Kara <jack@suse.cz>
>> Date: Mon, 27 Sep 2010 23:56:48 +0200
>> Subject: [PATCH] bdi: Initialize inode->i_mapping.backing_dev_info to sb->s_bdi
> ...
>> That works for me as well. Was it decided how to solve this? Other wise
>> I'll need to patch exofs, ASAP for this -rc
>   In the end, we'll use Christoph's patch
> (http://lkml.org/lkml/2010/9/29/76) changing inode_to_bdi() to be
> more conservative and also the warning will be gone. So you don't have to
> patch anything...
> 
> 								Honza

I would still like to fix it. Currently each inode->mapping.backing_dev_info in my
none-block-filesystem is set to &default_backing_dev_info. This sounds scary!
what about the future patches that will schedule a wakup on set_inode_dirty ?
Will they not need my proper sb->s_bdi on each ->mapping?

I could do it in the filesystem, but the way the code is now I'll need to
set it in 5 different places, or clean up the code with more common code.

That said, I think your (or my) patch makes much more sense. The sb->s_bdi
is a much better common default then &default_backing_dev_info. By now
is &default_backing_dev_info really needed at all?

I guess I'll have to go head and do it in FS code.

Thanks
Boaz

Re: [RFC] vfs/inode: For none-block-based filesystems default to sb->s_bdi

[Boaz Harrosh]

On 10/05/2010 09:53 AM, Boaz Harrosh wrote:
> On 10/05/2010 04:32 AM, Jan Kara wrote:
>>   Hi Boaz,
>>
>> On Mon 04-10-10 18:02:13, Boaz Harrosh wrote:
>>> Sorry I've just seen Jan's patch:
>>> From: Jan Kara <jack@suse.cz>
>>> Date: Mon, 27 Sep 2010 23:56:48 +0200
>>> Subject: [PATCH] bdi: Initialize inode->i_mapping.backing_dev_info to sb->s_bdi
>> ...
>>> That works for me as well. Was it decided how to solve this? Other wise
>>> I'll need to patch exofs, ASAP for this -rc
>>   In the end, we'll use Christoph's patch
>> (http://lkml.org/lkml/2010/9/29/76) changing inode_to_bdi() to be
>> more conservative and also the warning will be gone. So you don't have to
>> patch anything...
>>
>> 								Honza
> 
> I would still like to fix it. Currently each inode->mapping.backing_dev_info in my
> none-block-filesystem is set to &default_backing_dev_info. This sounds scary!
> what about the future patches that will schedule a wakup on set_inode_dirty ?
> Will they not need my proper sb->s_bdi on each ->mapping?
> 
> I could do it in the filesystem, but the way the code is now I'll need to
> set it in 5 different places, or clean up the code with more common code.
> 
> That said, I think your (or my) patch makes much more sense. The sb->s_bdi
> is a much better common default then &default_backing_dev_info. By now
> is &default_backing_dev_info really needed at all?
> 
> I guess I'll have to go head and do it in FS code.
> 

BTW: I liked that WARN_ON it exposed a real problem.

> Thanks
> Boaz

Re: [RFC] vfs/inode: For none-block-based filesystems default to sb->s_bdi

[Jan Kara]

On Tue 05-10-10 09:53:41, Boaz Harrosh wrote:
> On 10/05/2010 04:32 AM, Jan Kara wrote:
> >   Hi Boaz,
> > 
> > On Mon 04-10-10 18:02:13, Boaz Harrosh wrote:
> >> Sorry I've just seen Jan's patch:
> >> From: Jan Kara <jack@suse.cz>
> >> Date: Mon, 27 Sep 2010 23:56:48 +0200
> >> Subject: [PATCH] bdi: Initialize inode->i_mapping.backing_dev_info to sb->s_bdi
> > ...
> >> That works for me as well. Was it decided how to solve this? Other wise
> >> I'll need to patch exofs, ASAP for this -rc
> >   In the end, we'll use Christoph's patch
> > (http://lkml.org/lkml/2010/9/29/76) changing inode_to_bdi() to be
> > more conservative and also the warning will be gone. So you don't have to
> > patch anything...
> > 
> I would still like to fix it. Currently each inode->mapping.backing_dev_info in my
> none-block-filesystem is set to &default_backing_dev_info. This sounds scary!
> what about the future patches that will schedule a wakup on set_inode_dirty ?
> Will they not need my proper sb->s_bdi on each ->mapping?
  Yes, but Christoph is working on separating the bdi handling from
handling of writeback (pointed from superblock) and that should solve this
problem as well.

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

Re: [RFC] vfs/inode: For none-block-based filesystems default to sb->s_bdi

[Jan Kara]

On Tue 05-10-10 09:54:49, Boaz Harrosh wrote:
> On 10/05/2010 09:53 AM, Boaz Harrosh wrote:
> > On 10/05/2010 04:32 AM, Jan Kara wrote:
> >>   Hi Boaz,
> >>
> >> On Mon 04-10-10 18:02:13, Boaz Harrosh wrote:
> >>> Sorry I've just seen Jan's patch:
> >>> From: Jan Kara <jack@suse.cz>
> >>> Date: Mon, 27 Sep 2010 23:56:48 +0200
> >>> Subject: [PATCH] bdi: Initialize inode->i_mapping.backing_dev_info to sb->s_bdi
> >> ...
> >>> That works for me as well. Was it decided how to solve this? Other wise
> >>> I'll need to patch exofs, ASAP for this -rc
> >>   In the end, we'll use Christoph's patch
> >> (http://lkml.org/lkml/2010/9/29/76) changing inode_to_bdi() to be
> >> more conservative and also the warning will be gone. So you don't have to
> >> patch anything...
> >>
> >> 								Honza
> > 
> > I would still like to fix it. Currently each inode->mapping.backing_dev_info in my
> > none-block-filesystem is set to &default_backing_dev_info. This sounds scary!
> > what about the future patches that will schedule a wakup on set_inode_dirty ?
> > Will they not need my proper sb->s_bdi on each ->mapping?
> > 
> > I could do it in the filesystem, but the way the code is now I'll need to
> > set it in 5 different places, or clean up the code with more common code.
> > 
> > That said, I think your (or my) patch makes much more sense. The sb->s_bdi
> > is a much better common default then &default_backing_dev_info. By now
> > is &default_backing_dev_info really needed at all?
> > 
> > I guess I'll have to go head and do it in FS code.
> > 
> 
> BTW: I liked that WARN_ON it exposed a real problem.
  In fact, it exposed too many of them for being so late in the -rc cycle
;). Luckily they were all harmless (at least currently). So for now we are
better off without the warning so that we don't scare users.

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

Re: [RFC] vfs/inode: For none-block-based filesystems default to sb->s_bdi

[Boaz Harrosh]

On 10/05/2010 11:09 AM, Jan Kara wrote:
> On Tue 05-10-10 09:53:41, Boaz Harrosh wrote:
>> On 10/05/2010 04:32 AM, Jan Kara wrote:
>>>   Hi Boaz,
>>>
>>> On Mon 04-10-10 18:02:13, Boaz Harrosh wrote:
>>>> Sorry I've just seen Jan's patch:
>>>> From: Jan Kara <jack@suse.cz>
>>>> Date: Mon, 27 Sep 2010 23:56:48 +0200
>>>> Subject: [PATCH] bdi: Initialize inode->i_mapping.backing_dev_info to sb->s_bdi
>>> ...
>>>> That works for me as well. Was it decided how to solve this? Other wise
>>>> I'll need to patch exofs, ASAP for this -rc
>>>   In the end, we'll use Christoph's patch
>>> (http://lkml.org/lkml/2010/9/29/76) changing inode_to_bdi() to be
>>> more conservative and also the warning will be gone. So you don't have to
>>> patch anything...
>>>
>> I would still like to fix it. Currently each inode->mapping.backing_dev_info in my
>> none-block-filesystem is set to &default_backing_dev_info. This sounds scary!
>> what about the future patches that will schedule a wakup on set_inode_dirty ?
>> Will they not need my proper sb->s_bdi on each ->mapping?
>   Yes, but Christoph is working on separating the bdi handling from
> handling of writeback (pointed from superblock) and that should solve this
> problem as well.
> 

If you are right and inode->i_mapping.backing_dev_info will only be used for bdev
inodes, then I prefer if it would be NULL by default and not pointing to
&default_backing_dev_info. Will that be fixed as well?

diff --git a/fs/inode.c b/fs/inode.c
index 8646433..0ec4c47 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -168,7 +168,7 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
 	mapping->flags = 0;
 	mapping_set_gfp_mask(mapping, GFP_HIGHUSER_MOVABLE);
 	mapping->assoc_mapping = NULL;
-	mapping->backing_dev_info = &default_backing_dev_info;
+	mapping->backing_dev_info = NULL;
 	mapping->writeback_index = 0;
 
 	/*

Current code in inode_init_always() looks really ugly with that if(sb->s_bdev) ...
surely you want to fix that too, No?

> 								Honza

Boaz

Re: [RFC] vfs/inode: For none-block-based filesystems default to sb->s_bdi

[Jan Kara]

On Tue 05-10-10 11:29:55, Boaz Harrosh wrote:
> On 10/05/2010 11:09 AM, Jan Kara wrote:
> > On Tue 05-10-10 09:53:41, Boaz Harrosh wrote:
> >> On 10/05/2010 04:32 AM, Jan Kara wrote:
> >>>   Hi Boaz,
> >>>
> >>> On Mon 04-10-10 18:02:13, Boaz Harrosh wrote:
> >>>> Sorry I've just seen Jan's patch:
> >>>> From: Jan Kara <jack@suse.cz>
> >>>> Date: Mon, 27 Sep 2010 23:56:48 +0200
> >>>> Subject: [PATCH] bdi: Initialize inode->i_mapping.backing_dev_info to sb->s_bdi
> >>> ...
> >>>> That works for me as well. Was it decided how to solve this? Other wise
> >>>> I'll need to patch exofs, ASAP for this -rc
> >>>   In the end, we'll use Christoph's patch
> >>> (http://lkml.org/lkml/2010/9/29/76) changing inode_to_bdi() to be
> >>> more conservative and also the warning will be gone. So you don't have to
> >>> patch anything...
> >>>
> >> I would still like to fix it. Currently each inode->mapping.backing_dev_info in my
> >> none-block-filesystem is set to &default_backing_dev_info. This sounds scary!
> >> what about the future patches that will schedule a wakup on set_inode_dirty ?
> >> Will they not need my proper sb->s_bdi on each ->mapping?
> >   Yes, but Christoph is working on separating the bdi handling from
> > handling of writeback (pointed from superblock) and that should solve this
> > problem as well.
> > 
> 
> If you are right and inode->i_mapping.backing_dev_info will only be used for bdev
> inodes, then I prefer if it would be NULL by default and not pointing to
> &default_backing_dev_info. Will that be fixed as well?
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index 8646433..0ec4c47 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -168,7 +168,7 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
>  	mapping->flags = 0;
>  	mapping_set_gfp_mask(mapping, GFP_HIGHUSER_MOVABLE);
>  	mapping->assoc_mapping = NULL;
> -	mapping->backing_dev_info = &default_backing_dev_info;
> +	mapping->backing_dev_info = NULL;
>  	mapping->writeback_index = 0;
>  
>  	/*
> 
> Current code in inode_init_always() looks really ugly with that if(sb->s_bdev) ...
> surely you want to fix that too, No?
  Well, for now I'm happy that that can of worms has been closed with
Christoph's patch and I'll look into it again when Christoph comes with
his rework... Then we can do also cleanups like you suggest above.

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

Re: [RFC] vfs/inode: For none-block-based filesystems default to sb->s_bdi

[Christoph Hellwig]

On Tue, Oct 05, 2010 at 11:29:55AM -0400, Boaz Harrosh wrote:
> If you are right and inode->i_mapping.backing_dev_info will only be used for bdev
> inodes, then I prefer if it would be NULL by default and not pointing to
> &default_backing_dev_info. Will that be fixed as well?

It's been pointing to default_backing_dev_info since long before we got
the per-bdi writeback.  Once the writeback bits are're back to what we used
the BDI for before:

 - set VM readahead parameters
 - deal with per-backingdev congestion/unplugging
 - a couple of VM statistics

in general all these really should point to the same for a normal filesystem,
and at least point to the same in a group even for "strange" filesystems
that have multiple backing devices.  The BDI still is a rather strangely
defined beast, and so far all my attempts to make sense of it have failed.