* [RFC] vfs/inode: For none-block-based filesystems default to sb->s_bdi @ 2010-10-04 21:46 Boaz Harrosh 2010-10-04 22:02 ` Boaz Harrosh 0 siblings, 1 reply; 10+ messages in thread From: Boaz Harrosh @ 2010-10-04 21:46 UTC (permalink / raw) To: Jan Kara, Christoph Hellwig, Al Viro, linux-fsdevel Cc: Trond Myklebust, Benny Halevy 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC] vfs/inode: For none-block-based filesystems default to sb->s_bdi 2010-10-04 21:46 [RFC] vfs/inode: For none-block-based filesystems default to sb->s_bdi Boaz Harrosh @ 2010-10-04 22:02 ` Boaz Harrosh 2010-10-05 8:32 ` Jan Kara 0 siblings, 1 reply; 10+ messages in thread From: Boaz Harrosh @ 2010-10-04 22:02 UTC (permalink / raw) To: Jan Kara, Christoph Hellwig, Al Viro, linux-fsdevel Cc: Trond Myklebust, Benny Halevy 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC] vfs/inode: For none-block-based filesystems default to sb->s_bdi 2010-10-04 22:02 ` Boaz Harrosh @ 2010-10-05 8:32 ` Jan Kara 2010-10-05 13:53 ` Boaz Harrosh 0 siblings, 1 reply; 10+ messages in thread From: Jan Kara @ 2010-10-05 8:32 UTC (permalink / raw) To: Boaz Harrosh Cc: Jan Kara, Christoph Hellwig, Al Viro, linux-fsdevel, Trond Myklebust, Benny Halevy 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC] vfs/inode: For none-block-based filesystems default to sb->s_bdi 2010-10-05 8:32 ` Jan Kara @ 2010-10-05 13:53 ` Boaz Harrosh 2010-10-05 13:54 ` Boaz Harrosh 2010-10-05 15:09 ` Jan Kara 0 siblings, 2 replies; 10+ messages in thread From: Boaz Harrosh @ 2010-10-05 13:53 UTC (permalink / raw) To: Jan Kara Cc: Christoph Hellwig, Al Viro, linux-fsdevel, Trond Myklebust, Benny Halevy 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC] vfs/inode: For none-block-based filesystems default to sb->s_bdi 2010-10-05 13:53 ` Boaz Harrosh @ 2010-10-05 13:54 ` Boaz Harrosh 2010-10-05 15:12 ` Jan Kara 2010-10-05 15:09 ` Jan Kara 1 sibling, 1 reply; 10+ messages in thread From: Boaz Harrosh @ 2010-10-05 13:54 UTC (permalink / raw) To: Jan Kara Cc: Christoph Hellwig, Al Viro, linux-fsdevel, Trond Myklebust, Benny Halevy 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC] vfs/inode: For none-block-based filesystems default to sb->s_bdi 2010-10-05 13:54 ` Boaz Harrosh @ 2010-10-05 15:12 ` Jan Kara 0 siblings, 0 replies; 10+ messages in thread From: Jan Kara @ 2010-10-05 15:12 UTC (permalink / raw) To: Boaz Harrosh Cc: Jan Kara, Christoph Hellwig, Al Viro, linux-fsdevel, Trond Myklebust, Benny Halevy 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC] vfs/inode: For none-block-based filesystems default to sb->s_bdi 2010-10-05 13:53 ` Boaz Harrosh 2010-10-05 13:54 ` Boaz Harrosh @ 2010-10-05 15:09 ` Jan Kara 2010-10-05 15:29 ` Boaz Harrosh 1 sibling, 1 reply; 10+ messages in thread From: Jan Kara @ 2010-10-05 15:09 UTC (permalink / raw) To: Boaz Harrosh Cc: Jan Kara, Christoph Hellwig, Al Viro, linux-fsdevel, Trond Myklebust, Benny Halevy 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC] vfs/inode: For none-block-based filesystems default to sb->s_bdi 2010-10-05 15:09 ` Jan Kara @ 2010-10-05 15:29 ` Boaz Harrosh 2010-10-05 15:50 ` Jan Kara 2010-10-06 8:55 ` Christoph Hellwig 0 siblings, 2 replies; 10+ messages in thread From: Boaz Harrosh @ 2010-10-05 15:29 UTC (permalink / raw) To: Jan Kara Cc: Christoph Hellwig, Al Viro, linux-fsdevel, Trond Myklebust, Benny Halevy 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC] vfs/inode: For none-block-based filesystems default to sb->s_bdi 2010-10-05 15:29 ` Boaz Harrosh @ 2010-10-05 15:50 ` Jan Kara 2010-10-06 8:55 ` Christoph Hellwig 1 sibling, 0 replies; 10+ messages in thread From: Jan Kara @ 2010-10-05 15:50 UTC (permalink / raw) To: Boaz Harrosh Cc: Jan Kara, Christoph Hellwig, Al Viro, linux-fsdevel, Trond Myklebust, Benny Halevy 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC] vfs/inode: For none-block-based filesystems default to sb->s_bdi 2010-10-05 15:29 ` Boaz Harrosh 2010-10-05 15:50 ` Jan Kara @ 2010-10-06 8:55 ` Christoph Hellwig 1 sibling, 0 replies; 10+ messages in thread From: Christoph Hellwig @ 2010-10-06 8:55 UTC (permalink / raw) To: Boaz Harrosh Cc: Jan Kara, Christoph Hellwig, Al Viro, linux-fsdevel, Trond Myklebust, Benny Halevy 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2010-10-06 20:33 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-10-04 21:46 [RFC] vfs/inode: For none-block-based filesystems default to sb->s_bdi Boaz Harrosh 2010-10-04 22:02 ` Boaz Harrosh 2010-10-05 8:32 ` Jan Kara 2010-10-05 13:53 ` Boaz Harrosh 2010-10-05 13:54 ` Boaz Harrosh 2010-10-05 15:12 ` Jan Kara 2010-10-05 15:09 ` Jan Kara 2010-10-05 15:29 ` Boaz Harrosh 2010-10-05 15:50 ` Jan Kara 2010-10-06 8:55 ` Christoph Hellwig
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).