[PATCH] exofs: stop using s_dirt

[PATCH] exofs: stop using s_dirt

[Artem Bityutskiy]

From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

Exofs has the '->write_super()' handler and makes some use of the '->s_dirt'
superblock flag, but it really needs neither of them because it never sets
's_dirt' to one which means the VFS never calls its '->write_super()' handler.
Thus, remove both.

Note, I am trying to remove both 's_dirt' and 'write_super()' from VFS
altogether once all users are gone.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 fs/exofs/super.c |   11 -----------
 1 files changed, 0 insertions(+), 11 deletions(-)

diff --git a/fs/exofs/super.c b/fs/exofs/super.c
index 735ca06..6e1c515 100644
--- a/fs/exofs/super.c
+++ b/fs/exofs/super.c
@@ -400,8 +400,6 @@ static int exofs_sync_fs(struct super_block *sb, int wait)
 	ret = ore_write(ios);
 	if (unlikely(ret))
 		EXOFS_ERR("%s: ore_write failed.\n", __func__);
-	else
-		sb->s_dirt = 0;
 
 
 	unlock_super(sb);
@@ -412,14 +410,6 @@ out:
 	return ret;
 }
 
-static void exofs_write_super(struct super_block *sb)
-{
-	if (!(sb->s_flags & MS_RDONLY))
-		exofs_sync_fs(sb, 1);
-	else
-		sb->s_dirt = 0;
-}
-
 static void _exofs_print_device(const char *msg, const char *dev_path,
 				struct osd_dev *od, u64 pid)
 {
@@ -942,7 +932,6 @@ static const struct super_operations exofs_sops = {
 	.write_inode    = exofs_write_inode,
 	.evict_inode    = exofs_evict_inode,
 	.put_super      = exofs_put_super,
-	.write_super    = exofs_write_super,
 	.sync_fs	= exofs_sync_fs,
 	.statfs         = exofs_statfs,
 };
-- 
1.7.7.6

Re: [PATCH] exofs: stop using s_dirt

[Boaz Harrosh]

On 06/04/2012 02:48 PM, Artem Bityutskiy wrote:

> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> 
> Exofs has the '->write_super()' handler and makes some use of the '->s_dirt'
> superblock flag, but it really needs neither of them because it never sets
> 's_dirt' to one which means the VFS never calls its '->write_super()' handler.
> Thus, remove both.
> 


Thanks Artem. I have seen your other FS conversions and thought I would
eventually need to also do it for exofs, thanks for beating me to it ;-)

I have removed all uses of sb->s_dirt = 1 cases around last year, by
writing the SB-info as part of the create/delete commands directly. So
I agree it is not needed anymore see here
	1cea312 exofs: Write sbi->s_nextid as part of the Create command

I have one question though, which I did not understand at the time?

Today at exofs_write_super() we call exofs_sync_fs() (super_operations->sync_fs)
Who/when calls ->sync_fs() without the ->write_super() below.

But otherwise I agree that sb->s_dirt = 1 and ->write_super() are not needed
at all, for regular operations.

Should I take this for 3.6 through my tree. Or do you want my:

Ack-by: Boaz Harrosh <bharrosh@panasas.com>

> Note, I am trying to remove both 's_dirt' and 'write_super()' from VFS
> altogether once all users are gone.
> 
> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> ---
>  fs/exofs/super.c |   11 -----------
>  1 files changed, 0 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/exofs/super.c b/fs/exofs/super.c
> index 735ca06..6e1c515 100644
> --- a/fs/exofs/super.c
> +++ b/fs/exofs/super.c
> @@ -400,8 +400,6 @@ static int exofs_sync_fs(struct super_block *sb, int wait)
>  	ret = ore_write(ios);
>  	if (unlikely(ret))
>  		EXOFS_ERR("%s: ore_write failed.\n", __func__);
> -	else
> -		sb->s_dirt = 0;
>  
>  
>  	unlock_super(sb);
> @@ -412,14 +410,6 @@ out:
>  	return ret;
>  }
>  
> -static void exofs_write_super(struct super_block *sb)
> -{
> -	if (!(sb->s_flags & MS_RDONLY))
> -		exofs_sync_fs(sb, 1);
> -	else
> -		sb->s_dirt = 0;
> -}
> -
>  static void _exofs_print_device(const char *msg, const char *dev_path,
>  				struct osd_dev *od, u64 pid)
>  {
> @@ -942,7 +932,6 @@ static const struct super_operations exofs_sops = {
>  	.write_inode    = exofs_write_inode,
>  	.evict_inode    = exofs_evict_inode,
>  	.put_super      = exofs_put_super,
> -	.write_super    = exofs_write_super,
>  	.sync_fs	= exofs_sync_fs,
>  	.statfs         = exofs_statfs,
>  };

Re: [PATCH] exofs: stop using s_dirt

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 1567 bytes --]

On Mon, 2012-06-04 at 19:12 +0300, Boaz Harrosh wrote:
> I have one question though, which I did not understand at the time?
> 
> Today at exofs_write_super() we call exofs_sync_fs() (super_operations->sync_fs)
> Who/when calls ->sync_fs() without the ->write_super() below.

Not sure I understood your question correctly, but:

o VFS calls '->sync_fs()' on
  * sync(2)
  * syncfs(2)
  * when unmounting, before calling '->put_super()'
  * when re-mounting, before calling '->remount_fs()'
  * when freezing (happens when suspending the system)
  * when the underlying block device is synced (see 'fsync_bdev()')
o '->write_super()' is called only from one place - from the
  'sync_supers()' function (which is only used by the 'sync_supers'
   kernel thread) if the superblock is dirty. This thread wakes up every
   5 seconds and calls '->write_super()' for all dirty superblocks.

In case of exofs you never set 's_dirt' to non-zero, which means that
'exofs_write_super()' is never called. This means we can remove it and
this won't change anything for exofs.

Basically, in the ideal world where power-cuts and unclean reboots do
not exist, '->write_super()' is not needed because the superblock will
be synced on unmount. The 'write_super()' stuff is about making sure
that your dirty superblock stays dirty only for limited amount of time
(defined via /proc/sys/vm/dirty_writeback_centisecs) and then gets
synced so you have less chances end up with a not completely up-to-date
superblock.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] exofs: stop using s_dirt

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 265 bytes --]

On Mon, 2012-06-04 at 19:12 +0300, Boaz Harrosh wrote:
> Should I take this for 3.6 through my tree. Or do you want my:

Forgot to answer this question, sorry: if it is fine with you, please,
merge it through your tree.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] exofs: stop using s_dirt

[Boaz Harrosh]

On 06/05/2012 01:35 PM, Artem Bityutskiy wrote:

> On Mon, 2012-06-04 at 19:12 +0300, Boaz Harrosh wrote:
>> Should I take this for 3.6 through my tree. Or do you want my:
> 
> Forgot to answer this question, sorry: if it is fine with you, please,
> merge it through your tree.
> 


I will. It should appear on linux-next, next time I push new patches
destined for 3.6 Kernel

I conducted some tests and you are absolutely right. There is no effect
it is just dead code.

Thanks again
Boaz

Re: [PATCH] exofs: stop using s_dirt

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 679 bytes --]

On Mon, 2012-06-04 at 14:48 +0300, Artem Bityutskiy wrote:
> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> 
> Exofs has the '->write_super()' handler and makes some use of the '->s_dirt'
> superblock flag, but it really needs neither of them because it never sets
> 's_dirt' to one which means the VFS never calls its '->write_super()' handler.
> Thus, remove both.
> 
> Note, I am trying to remove both 's_dirt' and 'write_super()' from VFS
> altogether once all users are gone.
> 
> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>

Al or Boaz, would you please consider picking this patch?

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] exofs: stop using s_dirt

[Boaz Harrosh]

On 06/21/2012 03:29 PM, Artem Bityutskiy wrote:

> On Mon, 2012-06-04 at 14:48 +0300, Artem Bityutskiy wrote:
>> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
>>
>> Exofs has the '->write_super()' handler and makes some use of the '->s_dirt'
>> superblock flag, but it really needs neither of them because it never sets
>> 's_dirt' to one which means the VFS never calls its '->write_super()' handler.
>> Thus, remove both.
>>
>> Note, I am trying to remove both 's_dirt' and 'write_super()' from VFS
>> altogether once all users are gone.
>>
>> Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> 
> Al or Boaz, would you please consider picking this patch?
> 


I will

Sorry for the delay was sick and am only now coming back on.

It should all be included for the next merge window.

Thanks for doing this.
Boaz