[PATCH 0/3] fs/sysv: stop using write_supers and s_dirt

[PATCH 0/3] fs/sysv: stop using write_supers and s_dirt

[Artem Bityutskiy]

This patch-set makes sysv file-system stop using the VFS '->write_supers()'
call-back and the '->s_dirt' superblock field because I plan to remove them
once all users are gone.

Compile-tested only. If someone can give me a sysv image I could loop-mount
and test my changes - I'd appreciate. I could not find any tool to create this
ancient file-system. But I think my changes should not break it.

Reminder:

The goal is to get rid of the 'sync_supers()' kernel thread. This kernel thread
wakes up every 5 seconds (by default) and calls '->write_super()' for all
mounted file-systems. And the bad thing is that this is done even if all the
superblocks are clean. Moreover, many file-systems do not even need this end
they do not register the '->write_super()' method at all (e.g., btrfs).

So 'sync_supers()' most often just generates useless wake-ups and wastes power.
I am trying to make all file-systems independent of '->write_super()' and plan
to remove 'sync_supers()' and '->write_super()' completely once there are no
more users.

======
Overall status:

1.  ext4: patches submitted,
    https://lkml.org/lkml/2012/7/3/210
2.  exofs: patch submitted,
    https://lkml.org/lkml/2012/6/4/211
3.  udf: patch submitted, sits in Jan Kara's tree:
    https://lkml.org/lkml/2012/6/4/233
    git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs for_testing
4.  affs: patches submitted, sit in Al Viro's tree:
    https://lkml.org/lkml/2012/6/6/400
    git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs for-next
5.  hfs: patches submitted, sit Andrew Morton's tree
    http://lkml.org/lkml/2012/6/12/82
6.  hfsplus: patches submitted, sit in Andre Morton's tree:
    https://lkml.org/lkml/2012/6/13/195
7.  ext2:     done, see commit f72cf5e223a28d3b3ea7dc9e40464fd534e359e8
8.  vfat:     done, see commit 78491189ddb6d84d4a4abae992ed891a236d0263
9.  jffs2:    done, see commit 208b14e507c00ff7f108e1a388dd3d8cc805a443
10. reiserfs: done, see commit 033369d1af1264abc23bea2e174aa47cdd212f6f

TODO: ufs
======

 fs/sysv/inode.c |   18 +-----------------
 fs/sysv/sysv.h  |    1 -
 2 files changed, 1 insertions(+), 18 deletions(-)

Thanks,
Artem.

[PATCH 1/3] fs/sysv: remove useless write_super call

[Artem Bityutskiy]

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

We do not need to call 'sysv_write_super()' from 'sysv_put_super()',
because VFS has called 'sysv_sync_fs()' before calling '->put_super()'.
So remove it.

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

diff --git a/fs/sysv/inode.c b/fs/sysv/inode.c
index 08d0b25..af13d13 100644
--- a/fs/sysv/inode.c
+++ b/fs/sysv/inode.c
@@ -81,9 +81,6 @@ static void sysv_put_super(struct super_block *sb)
 {
 	struct sysv_sb_info *sbi = SYSV_SB(sb);
 
-	if (sb->s_dirt)
-		sysv_write_super(sb);
-
 	if (!(sb->s_flags & MS_RDONLY)) {
 		/* XXX ext2 also updates the state here */
 		mark_buffer_dirty(sbi->s_bh1);
-- 
1.7.7.6

[PATCH 2/3] fs/sysv: remove another useless write_super call

[Artem Bityutskiy]

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

We do not need to call 'sysv_write_super()' from 'sysv_remount()',
because VFS has called 'sysv_sync_fs()' before calling '->remount()'.
So remove it. Remove also '(un)lock_super()' which obvioulsy is becoming
useless in this function.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 fs/sysv/inode.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/fs/sysv/inode.c b/fs/sysv/inode.c
index af13d13..f20ffe3 100644
--- a/fs/sysv/inode.c
+++ b/fs/sysv/inode.c
@@ -68,12 +68,9 @@ static void sysv_write_super(struct super_block *sb)
 static int sysv_remount(struct super_block *sb, int *flags, char *data)
 {
 	struct sysv_sb_info *sbi = SYSV_SB(sb);
-	lock_super(sb);
+
 	if (sbi->s_forced_ro)
 		*flags |= MS_RDONLY;
-	if (*flags & MS_RDONLY)
-		sysv_write_super(sb);
-	unlock_super(sb);
 	return 0;
 }
 
-- 
1.7.7.6

[PATCH 3/3] fs/sysv: stop using write_super and s_dirt

[Artem Bityutskiy]

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

It does not look like sysv FS needs 'write_super()' at all, because all it
does is a timestamp update. I cannot test this patch, because this
file-system is so old and probably has not been used by anyone for years,
so there are no tools to create it in Linux. But from the code I see that
marking the superblock as dirty is basically marking the superblock buffers as
drity and then setting the s_dirt flag. And when 'write_super()' is executed to
handle the s_dirt flag, we just update the timestamp and again mark the
superblock buffer as dirty. Seems pointless.

It looks like we can update the timestamp more opprtunistically - on unmount
or remount of sync, and nothing should change.

Thus, this patch removes 'sysv_write_super()' and 's_dirt'.

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

diff --git a/fs/sysv/inode.c b/fs/sysv/inode.c
index f20ffe3..80e1e2b 100644
--- a/fs/sysv/inode.c
+++ b/fs/sysv/inode.c
@@ -43,7 +43,6 @@ static int sysv_sync_fs(struct super_block *sb, int wait)
 	 * then attach current time stamp.
 	 * But if the filesystem was marked clean, keep it clean.
 	 */
-	sb->s_dirt = 0;
 	old_time = fs32_to_cpu(sbi, *sbi->s_sb_time);
 	if (sbi->s_type == FSTYPE_SYSV4) {
 		if (*sbi->s_sb_state == cpu_to_fs32(sbi, 0x7c269d38 - old_time))
@@ -57,14 +56,6 @@ static int sysv_sync_fs(struct super_block *sb, int wait)
 	return 0;
 }
 
-static void sysv_write_super(struct super_block *sb)
-{
-	if (!(sb->s_flags & MS_RDONLY))
-		sysv_sync_fs(sb, 1);
-	else
-		sb->s_dirt = 0;
-}
-
 static int sysv_remount(struct super_block *sb, int *flags, char *data)
 {
 	struct sysv_sb_info *sbi = SYSV_SB(sb);
@@ -351,7 +342,6 @@ const struct super_operations sysv_sops = {
 	.write_inode	= sysv_write_inode,
 	.evict_inode	= sysv_evict_inode,
 	.put_super	= sysv_put_super,
-	.write_super	= sysv_write_super,
 	.sync_fs	= sysv_sync_fs,
 	.remount_fs	= sysv_remount,
 	.statfs		= sysv_statfs,
diff --git a/fs/sysv/sysv.h b/fs/sysv/sysv.h
index 11b0767..0bc35fd 100644
--- a/fs/sysv/sysv.h
+++ b/fs/sysv/sysv.h
@@ -117,7 +117,6 @@ static inline void dirty_sb(struct super_block *sb)
 	mark_buffer_dirty(sbi->s_bh1);
 	if (sbi->s_bh1 != sbi->s_bh2)
 		mark_buffer_dirty(sbi->s_bh2);
-	sb->s_dirt = 1;
 }
 
 
-- 
1.7.7.6

Re: [PATCH 3/3] fs/sysv: stop using write_super and s_dirt

[Alan Cox]

> drity and then setting the s_dirt flag. And when 'write_super()' is executed to
> handle the s_dirt flag, we just update the timestamp and again mark the
> superblock buffer as dirty. Seems pointless.

It's the way sysv itself does stuff. The timestamp on v7/sys3/sys5
reflects the clock on the box. A lot of ancient boxes with no decent
hardware clocks used to reset the system time off the root fs superblock
on a loss of power.

Alan

Re: [PATCH 3/3] fs/sysv: stop using write_super and s_dirt

[Artem Bityutskiy]

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

On Tue, 2012-07-03 at 15:13 +0100, Alan Cox wrote:
> > drity and then setting the s_dirt flag. And when 'write_super()' is executed to
> > handle the s_dirt flag, we just update the timestamp and again mark the
> > superblock buffer as dirty. Seems pointless.
> 
> It's the way sysv itself does stuff. The timestamp on v7/sys3/sys5
> reflects the clock on the box. A lot of ancient boxes with no decent
> hardware clocks used to reset the system time off the root fs superblock
> on a loss of power.

Oh, OK, thanks for the info. Do you think I need to take more efforts to
preserve the behavior?

-- 
Best Regards,
Artem Bityutskiy

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

Re: [PATCH 0/3] fs/sysv: stop using write_supers and s_dirt

[Artem Bityutskiy]

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

On Tue, 2012-07-03 at 16:43 +0300, Artem Bityutskiy wrote:
> This patch-set makes sysv file-system stop using the VFS '->write_supers()'
> call-back and the '->s_dirt' superblock field because I plan to remove them
> once all users are gone.

Hi Andrew,

would you please pick this patch-set as well? I also sent you an update
for hfs and hfsplus file-systems today - found a bug while doing some
more testing. Also, I sent UFS changes today, very similar. Most
probably no one will reply and I hope you would pick them as well.

Thanks a lot!

-- 
Best Regards,
Artem Bityutskiy

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

Re: [PATCH 0/3] fs/sysv: stop using write_supers and s_dirt

[Artem Bityutskiy]

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

On Thu, 2012-07-12 at 17:37 +0300, Artem Bityutskiy wrote:
> On Tue, 2012-07-03 at 16:43 +0300, Artem Bityutskiy wrote:
> > This patch-set makes sysv file-system stop using the VFS '->write_supers()'
> > call-back and the '->s_dirt' superblock field because I plan to remove them
> > once all users are gone.
> 
> Hi Andrew,
> 
> would you please pick this patch-set as well? I also sent you an update
> for hfs and hfsplus file-systems today - found a bug while doing some
> more testing. Also, I sent UFS changes today, very similar. Most
> probably no one will reply and I hope you would pick them as well.

Just to let you know about the overall picture. Pending FSes (no one
picked them) are:

1. ext4 - Ted is still being busy.
2. sysv, ufs - I hope you'd pick

After this we will be able to kill sync_supers() completely and get rid
of another source of regular wake-ups in the kernel. And interestingly,
it will be the final end of pdflush - at old times it woke up
periodically and flushed everything (forked itself as well). Then Jens
cut off the data write-back part and put it to per-block device bdi
threads. They originally also waked-up every 5 seconds, but I fixed this
in v2.6.36 (6467716) and they stopped misbehaving. Jens transformed the
superblock write-out part of pdflush 'sync_supers()', and I could not
kill it because I did not have enough time and Al's request to change
all users sounded unrealistic to me. But now I hope to actually finish
this.

-- 
Best Regards,
Artem Bityutskiy

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

Re: [PATCH 0/3] fs/sysv: stop using write_supers and s_dirt

[Andrew Morton]

On Thu, 12 Jul 2012 17:37:58 +0300
Artem Bityutskiy <dedekind1@gmail.com> wrote:

> On Tue, 2012-07-03 at 16:43 +0300, Artem Bityutskiy wrote:
> > This patch-set makes sysv file-system stop using the VFS '->write_supers()'
> > call-back and the '->s_dirt' superblock field because I plan to remove them
> > once all users are gone.
> 
> Hi Andrew,
> 
> would you please pick this patch-set as well? I also sent you an update
> for hfs and hfsplus file-systems today - found a bug while doing some
> more testing. Also, I sent UFS changes today, very similar. Most
> probably no one will reply and I hope you would pick them as well.
> 

The issue Alan raised around the superblock timestamp is still up in
the air.  I guess he's a slow typist ;)

My take is "no, we don't need to do that any more" - surely all Linux
systems have a functional hardware clock.  But the changelog should be
updated to describe and justify the decision.

Re: [PATCH 0/3] fs/sysv: stop using write_supers and s_dirt

[Artem Bityutskiy]

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

On Fri, 2012-07-13 at 14:42 -0700, Andrew Morton wrote:
> 
> The issue Alan raised around the superblock timestamp is still up in
> the air.  I guess he's a slow typist ;)
> 
> My take is "no, we don't need to do that any more" - surely all Linux
> systems have a functional hardware clock.  But the changelog should be
> updated to describe and justify the decision.
> 
While I do trust such system existed and may be even still exist, I
doubt that Linux sysv FS implementation is of any help for them because
it updates the superblock time-stamp _only_ if there was write activity,
otherwise it does not. So you cannot rely on our time-stamps at all
anyway. My patches just make it update the time-stamp more rarely.

-- 
Best Regards,
Artem Bityutskiy

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

Re: [PATCH 0/3] fs/sysv: stop using write_supers and s_dirt

[Artem Bityutskiy]

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

From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Date: Tue, 3 Jul 2012 15:47:44 +0300
Subject: [PATCH] fs/sysv: stop using write_super and s_dirt

It does not look like sysv FS needs 'write_super()' at all, because all it
does is a timestamp update. I cannot test this patch, because this
file-system is so old and probably has not been used by anyone for years,
so there are no tools to create it in Linux.

In sysv FS marking the superblock as dirty is basically marking the superblock
buffers as drity and then setting the s_dirt flag. And when 'write_super()' is
executed to handle the s_dirt flag, we just update the timestamp and again mark
the superblock buffer as dirty. The reason for this strange behavior is
probably that the Linux sysv implementation is trying to do what sysv itself
was doing:

"It's the way sysv itself does stuff. The timestamp on v7/sys3/sys5
reflects the clock on the box. A lot of ancient boxes with no decent
hardware clocks used to reset the system time off the root fs superblock
on a loss of power" (Alan Cox).

However, the Linux sysv FS implementation is very unlikely of much help for
clock-less systems anyway, because it updates the superblock time-stamp _only_
if there was write activity, therwise it does not.

Thus, we can update the timestamp more opprtunistically - on unmount or remount
of sync, and nothing should change. So this patch removes 'sysv_write_super()'
and 's_dirt'.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---

Updated the changelog, nothing else.

 fs/sysv/inode.c |   10 ----------
 fs/sysv/sysv.h  |    1 -
 2 files changed, 0 insertions(+), 11 deletions(-)

diff --git a/fs/sysv/inode.c b/fs/sysv/inode.c
index f20ffe3..80e1e2b 100644
--- a/fs/sysv/inode.c
+++ b/fs/sysv/inode.c
@@ -43,7 +43,6 @@ static int sysv_sync_fs(struct super_block *sb, int wait)
 	 * then attach current time stamp.
 	 * But if the filesystem was marked clean, keep it clean.
 	 */
-	sb->s_dirt = 0;
 	old_time = fs32_to_cpu(sbi, *sbi->s_sb_time);
 	if (sbi->s_type == FSTYPE_SYSV4) {
 		if (*sbi->s_sb_state == cpu_to_fs32(sbi, 0x7c269d38 - old_time))
@@ -57,14 +56,6 @@ static int sysv_sync_fs(struct super_block *sb, int wait)
 	return 0;
 }
 
-static void sysv_write_super(struct super_block *sb)
-{
-	if (!(sb->s_flags & MS_RDONLY))
-		sysv_sync_fs(sb, 1);
-	else
-		sb->s_dirt = 0;
-}
-
 static int sysv_remount(struct super_block *sb, int *flags, char *data)
 {
 	struct sysv_sb_info *sbi = SYSV_SB(sb);
@@ -351,7 +342,6 @@ const struct super_operations sysv_sops = {
 	.write_inode	= sysv_write_inode,
 	.evict_inode	= sysv_evict_inode,
 	.put_super	= sysv_put_super,
-	.write_super	= sysv_write_super,
 	.sync_fs	= sysv_sync_fs,
 	.remount_fs	= sysv_remount,
 	.statfs		= sysv_statfs,
diff --git a/fs/sysv/sysv.h b/fs/sysv/sysv.h
index 11b0767..0bc35fd 100644
--- a/fs/sysv/sysv.h
+++ b/fs/sysv/sysv.h
@@ -117,7 +117,6 @@ static inline void dirty_sb(struct super_block *sb)
 	mark_buffer_dirty(sbi->s_bh1);
 	if (sbi->s_bh1 != sbi->s_bh2)
 		mark_buffer_dirty(sbi->s_bh2);
-	sb->s_dirt = 1;
 }
 
 
-- 
1.7.7.6



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