All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] hfsplus: stop using write_supers and s_dirt
@ 2012-06-13 11:37 Artem Bityutskiy
  2012-06-13 11:37 ` [PATCH 1/4] hfsplus: make hfsplus_sync_fs static Artem Bityutskiy
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: Artem Bityutskiy @ 2012-06-13 11:37 UTC (permalink / raw)
  To: Al Viro; +Cc: Linux FS Maling List, Linux Kernel Maling List

This patch-set makes HFSPLUS 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.

Like the other similar patch-sets, we switch to a delayed job for writing out
the superblock instead of using the 's_dirt' flag. Additionally, this patch-set
includes several clean-ups.

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.

Al, in the past I was trying to upstream patches which optimized 'sync_super()',
but you wanted me to kill it completely instead, which I am trying to do
now, see http://lkml.org/lkml/2010/7/22/96

Tested using the fsstress test from the LTP project.

======
Overall status:

1.  ext4: patches submitted, waiting for review from Ted Ts'o:
    https://lkml.org/lkml/2012/4/2/111
2.  hfs: patches submitted, see
    http://lkml.org/lkml/2012/6/12/82
3.  hfsplus: this patch-set
4.  udf: patch submitted, should be in Jan Kara's tree:
    https://lkml.org/lkml/2012/6/4/233
5.  exofs: patch submitted, not sure if it will go to the exofs tree:
    https://lkml.org/lkml/2012/6/4/211
6.  affs: patches submitted, should be in Al Viro's tree:
    https://lkml.org/lkml/2012/6/6/400
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: sysv, ufs
======

 fs/hfsplus/bitmap.c     |    4 ++--
 fs/hfsplus/dir.c        |    2 +-
 fs/hfsplus/hfsplus_fs.h |    7 +++++--
 fs/hfsplus/inode.c      |    6 +++---
 fs/hfsplus/super.c      |   46 ++++++++++++++++++++++++++++++++++------------
 5 files changed, 45 insertions(+), 20 deletions(-)

Thanks,
Artem.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 1/4] hfsplus: make hfsplus_sync_fs static
  2012-06-13 11:37 [PATCH 0/4] hfsplus: stop using write_supers and s_dirt Artem Bityutskiy
@ 2012-06-13 11:37 ` Artem Bityutskiy
  2012-06-13 11:37 ` [PATCH 2/4] hfsplus: amend debugging print Artem Bityutskiy
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Artem Bityutskiy @ 2012-06-13 11:37 UTC (permalink / raw)
  To: Al Viro; +Cc: Linux FS Maling List, Linux Kernel Maling List

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

... because it is used only in fs/hfsplus/super.c.

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

diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
index 4e75ac6..66a9365 100644
--- a/fs/hfsplus/hfsplus_fs.h
+++ b/fs/hfsplus/hfsplus_fs.h
@@ -428,7 +428,6 @@ int hfsplus_show_options(struct seq_file *, struct dentry *);
 
 /* super.c */
 struct inode *hfsplus_iget(struct super_block *, unsigned long);
-int hfsplus_sync_fs(struct super_block *sb, int wait);
 
 /* tables.c */
 extern u16 hfsplus_case_fold_table[];
diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index a9bca4b..5df771e 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -161,7 +161,7 @@ static void hfsplus_evict_inode(struct inode *inode)
 	}
 }
 
-int hfsplus_sync_fs(struct super_block *sb, int wait)
+static int hfsplus_sync_fs(struct super_block *sb, int wait)
 {
 	struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
 	struct hfsplus_vh *vhdr = sbi->s_vhdr;
-- 
1.7.7.6


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/4] hfsplus: amend debugging print
  2012-06-13 11:37 [PATCH 0/4] hfsplus: stop using write_supers and s_dirt Artem Bityutskiy
  2012-06-13 11:37 ` [PATCH 1/4] hfsplus: make hfsplus_sync_fs static Artem Bityutskiy
@ 2012-06-13 11:37 ` Artem Bityutskiy
  2012-06-13 11:37 ` [PATCH 3/4] hfsplus: remove useless check Artem Bityutskiy
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Artem Bityutskiy @ 2012-06-13 11:37 UTC (permalink / raw)
  To: Al Viro; +Cc: Linux FS Maling List, Linux Kernel Maling List

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

Print correct function name in the debugging print of the
'hfsplus_sync_fs()' function.

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

diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index 5df771e..9e9c2788 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -171,7 +171,7 @@ static int hfsplus_sync_fs(struct super_block *sb, int wait)
 	if (!wait)
 		return 0;
 
-	dprint(DBG_SUPER, "hfsplus_write_super\n");
+	dprint(DBG_SUPER, "hfsplus_sync_fs\n");
 
 	sb->s_dirt = 0;
 
-- 
1.7.7.6


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 3/4] hfsplus: remove useless check
  2012-06-13 11:37 [PATCH 0/4] hfsplus: stop using write_supers and s_dirt Artem Bityutskiy
  2012-06-13 11:37 ` [PATCH 1/4] hfsplus: make hfsplus_sync_fs static Artem Bityutskiy
  2012-06-13 11:37 ` [PATCH 2/4] hfsplus: amend debugging print Artem Bityutskiy
@ 2012-06-13 11:37 ` Artem Bityutskiy
  2012-06-13 11:37 ` [PATCH 4/4] hfsplus: get rid of write_super Artem Bityutskiy
  2012-06-21 12:16 ` [PATCH 0/4] hfsplus: stop using write_supers and s_dirt Artem Bityutskiy
  4 siblings, 0 replies; 10+ messages in thread
From: Artem Bityutskiy @ 2012-06-13 11:37 UTC (permalink / raw)
  To: Al Viro; +Cc: Linux FS Maling List, Linux Kernel Maling List

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

This check is useless because we always have 'sb->s_fs_info' to be non-NULL.

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

diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index 9e9c2788..f4f3d54 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -240,9 +240,6 @@ static void hfsplus_put_super(struct super_block *sb)
 
 	dprint(DBG_SUPER, "hfsplus_put_super\n");
 
-	if (!sb->s_fs_info)
-		return;
-
 	if (!(sb->s_flags & MS_RDONLY) && sbi->s_vhdr) {
 		struct hfsplus_vh *vhdr = sbi->s_vhdr;
 
-- 
1.7.7.6


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 4/4] hfsplus: get rid of write_super
  2012-06-13 11:37 [PATCH 0/4] hfsplus: stop using write_supers and s_dirt Artem Bityutskiy
                   ` (2 preceding siblings ...)
  2012-06-13 11:37 ` [PATCH 3/4] hfsplus: remove useless check Artem Bityutskiy
@ 2012-06-13 11:37 ` Artem Bityutskiy
  2012-06-21 19:41   ` Andrew Morton
  2012-07-02 14:13   ` Artem Bityutskiy
  2012-06-21 12:16 ` [PATCH 0/4] hfsplus: stop using write_supers and s_dirt Artem Bityutskiy
  4 siblings, 2 replies; 10+ messages in thread
From: Artem Bityutskiy @ 2012-06-13 11:37 UTC (permalink / raw)
  To: Al Viro; +Cc: Linux FS Maling List, Linux Kernel Maling List

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

This patch makes hfsplus stop using the VFS '->write_super()' method along with
the 's_dirt' superblock flag, because they are on their way out.

The whole "superblock write-out" VFS infrastructure is served by the
'sync_supers()' kernel thread, which wakes up every 5 (by default) seconds and
writes out all dirty superblocks using the '->write_super()' call-back.  But the
problem with this thread is that it wastes power by waking up the system every
5 seconds, even if there are no diry superblocks, or there are no client
file-systems which would need this (e.g., btrfs does not use
'->write_super()'). So we want to kill it completely and thus, we need to make
file-systems to stop using the '->write_super()' VFS service, and then remove
it together with the kernel thread.

Tested using fsstress from the LTP project.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 fs/hfsplus/bitmap.c     |    4 ++--
 fs/hfsplus/dir.c        |    2 +-
 fs/hfsplus/hfsplus_fs.h |    6 +++++-
 fs/hfsplus/inode.c      |    6 +++---
 fs/hfsplus/super.c      |   41 +++++++++++++++++++++++++++++++++--------
 5 files changed, 44 insertions(+), 15 deletions(-)

diff --git a/fs/hfsplus/bitmap.c b/fs/hfsplus/bitmap.c
index 1cad80c..4cfbe2e 100644
--- a/fs/hfsplus/bitmap.c
+++ b/fs/hfsplus/bitmap.c
@@ -153,7 +153,7 @@ done:
 	kunmap(page);
 	*max = offset + (curr - pptr) * 32 + i - start;
 	sbi->free_blocks -= *max;
-	sb->s_dirt = 1;
+	hfsplus_mark_mdb_dirty(sb);
 	dprint(DBG_BITMAP, "-> %u,%u\n", start, *max);
 out:
 	mutex_unlock(&sbi->alloc_mutex);
@@ -228,7 +228,7 @@ out:
 	set_page_dirty(page);
 	kunmap(page);
 	sbi->free_blocks += len;
-	sb->s_dirt = 1;
+	hfsplus_mark_mdb_dirty(sb);
 	mutex_unlock(&sbi->alloc_mutex);
 
 	return 0;
diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c
index 26b53fb..8375504 100644
--- a/fs/hfsplus/dir.c
+++ b/fs/hfsplus/dir.c
@@ -316,7 +316,7 @@ static int hfsplus_link(struct dentry *src_dentry, struct inode *dst_dir,
 	inode->i_ctime = CURRENT_TIME_SEC;
 	mark_inode_dirty(inode);
 	sbi->file_count++;
-	dst_dir->i_sb->s_dirt = 1;
+	hfsplus_mark_mdb_dirty(dst_dir->i_sb);
 out:
 	mutex_unlock(&sbi->vh_mutex);
 	return res;
diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
index 66a9365..558dbb4 100644
--- a/fs/hfsplus/hfsplus_fs.h
+++ b/fs/hfsplus/hfsplus_fs.h
@@ -153,8 +153,11 @@ struct hfsplus_sb_info {
 	gid_t gid;
 
 	int part, session;
-
 	unsigned long flags;
+
+	int work_queued;               /* non-zero delayed work is queued */
+	struct delayed_work sync_work; /* FS sync delayed work */
+	spinlock_t work_lock;          /* protects sync_work and work_queued */
 };
 
 #define HFSPLUS_SB_WRITEBACKUP	0
@@ -428,6 +431,7 @@ int hfsplus_show_options(struct seq_file *, struct dentry *);
 
 /* super.c */
 struct inode *hfsplus_iget(struct super_block *, unsigned long);
+void hfsplus_mark_mdb_dirty(struct super_block *sb);
 
 /* tables.c */
 extern u16 hfsplus_case_fold_table[];
diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
index 82b69ee..ad1f16f 100644
--- a/fs/hfsplus/inode.c
+++ b/fs/hfsplus/inode.c
@@ -431,7 +431,7 @@ struct inode *hfsplus_new_inode(struct super_block *sb, umode_t mode)
 		sbi->file_count++;
 	insert_inode_hash(inode);
 	mark_inode_dirty(inode);
-	sb->s_dirt = 1;
+	hfsplus_mark_mdb_dirty(sb);
 
 	return inode;
 }
@@ -442,7 +442,7 @@ void hfsplus_delete_inode(struct inode *inode)
 
 	if (S_ISDIR(inode->i_mode)) {
 		HFSPLUS_SB(sb)->folder_count--;
-		sb->s_dirt = 1;
+		hfsplus_mark_mdb_dirty(sb);
 		return;
 	}
 	HFSPLUS_SB(sb)->file_count--;
@@ -455,7 +455,7 @@ void hfsplus_delete_inode(struct inode *inode)
 		inode->i_size = 0;
 		hfsplus_file_truncate(inode);
 	}
-	sb->s_dirt = 1;
+	hfsplus_mark_mdb_dirty(sb);
 }
 
 void hfsplus_inode_read_fork(struct inode *inode, struct hfsplus_fork_raw *fork)
diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index f4f3d54..cb6b9c1 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -124,7 +124,7 @@ static int hfsplus_system_write_inode(struct inode *inode)
 
 	if (fork->total_size != cpu_to_be64(inode->i_size)) {
 		set_bit(HFSPLUS_SB_WRITEBACKUP, &sbi->flags);
-		inode->i_sb->s_dirt = 1;
+		hfsplus_mark_mdb_dirty(inode->i_sb);
 	}
 	hfsplus_inode_write_fork(inode, fork);
 	if (tree)
@@ -173,7 +173,7 @@ static int hfsplus_sync_fs(struct super_block *sb, int wait)
 
 	dprint(DBG_SUPER, "hfsplus_sync_fs\n");
 
-	sb->s_dirt = 0;
+	cancel_delayed_work(&sbi->sync_work);
 
 	/*
 	 * Explicitly write out the special metadata inodes.
@@ -226,12 +226,34 @@ out:
 	return error;
 }
 
-static void hfsplus_write_super(struct super_block *sb)
+static void delayed_sync_fs(struct work_struct *work)
 {
-	if (!(sb->s_flags & MS_RDONLY))
-		hfsplus_sync_fs(sb, 1);
-	else
-		sb->s_dirt = 0;
+	struct hfsplus_sb_info *sbi;
+
+	sbi = container_of(work, struct hfsplus_sb_info, sync_work.work);
+
+	spin_lock(&sbi->work_lock);
+	sbi->work_queued = 0;
+	spin_unlock(&sbi->work_lock);
+
+	hfsplus_sync_fs(sbi->alloc_file->i_sb, 1);
+}
+
+void hfsplus_mark_mdb_dirty(struct super_block *sb)
+{
+	struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
+	unsigned long delay;
+
+	if (sb->s_flags & MS_RDONLY)
+	       return;
+
+	spin_lock(&sbi->work_lock);
+	if (!sbi->work_queued) {
+	       delay = msecs_to_jiffies(dirty_writeback_interval * 10);
+	       queue_delayed_work(system_long_wq, &sbi->sync_work, delay);
+	       sbi->work_queued = 1;
+	}
+	spin_unlock(&sbi->work_lock);
 }
 
 static void hfsplus_put_super(struct super_block *sb)
@@ -240,6 +262,8 @@ static void hfsplus_put_super(struct super_block *sb)
 
 	dprint(DBG_SUPER, "hfsplus_put_super\n");
 
+	cancel_delayed_work_sync(&sbi->sync_work);
+
 	if (!(sb->s_flags & MS_RDONLY) && sbi->s_vhdr) {
 		struct hfsplus_vh *vhdr = sbi->s_vhdr;
 
@@ -325,7 +349,6 @@ static const struct super_operations hfsplus_sops = {
 	.write_inode	= hfsplus_write_inode,
 	.evict_inode	= hfsplus_evict_inode,
 	.put_super	= hfsplus_put_super,
-	.write_super	= hfsplus_write_super,
 	.sync_fs	= hfsplus_sync_fs,
 	.statfs		= hfsplus_statfs,
 	.remount_fs	= hfsplus_remount,
@@ -352,6 +375,8 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent)
 	sb->s_fs_info = sbi;
 	mutex_init(&sbi->alloc_mutex);
 	mutex_init(&sbi->vh_mutex);
+	spin_lock_init(&sbi->work_lock);
+	INIT_DELAYED_WORK(&sbi->sync_work, delayed_sync_fs);
 	hfsplus_fill_defaults(sbi);
 
 	err = -EINVAL;
-- 
1.7.7.6


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 0/4] hfsplus: stop using write_supers and s_dirt
  2012-06-13 11:37 [PATCH 0/4] hfsplus: stop using write_supers and s_dirt Artem Bityutskiy
                   ` (3 preceding siblings ...)
  2012-06-13 11:37 ` [PATCH 4/4] hfsplus: get rid of write_super Artem Bityutskiy
@ 2012-06-21 12:16 ` Artem Bityutskiy
  4 siblings, 0 replies; 10+ messages in thread
From: Artem Bityutskiy @ 2012-06-21 12:16 UTC (permalink / raw)
  To: Al Viro, Andrew Morton; +Cc: Linux FS Maling List, Linux Kernel Maling List

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

On Wed, 2012-06-13 at 14:37 +0300, Artem Bityutskiy wrote:
> This patch-set makes HFSPLUS 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, Al or Andrew, would you please consider picking this patch-set?

-- 
Best Regards,
Artem Bityutskiy

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 4/4] hfsplus: get rid of write_super
  2012-06-13 11:37 ` [PATCH 4/4] hfsplus: get rid of write_super Artem Bityutskiy
@ 2012-06-21 19:41   ` Andrew Morton
  2012-06-21 20:30     ` Artem Bityutskiy
  2012-07-02 14:13   ` Artem Bityutskiy
  1 sibling, 1 reply; 10+ messages in thread
From: Andrew Morton @ 2012-06-21 19:41 UTC (permalink / raw)
  To: Artem Bityutskiy; +Cc: Al Viro, Linux FS Maling List, Linux Kernel Maling List

On Wed, 13 Jun 2012 14:37:51 +0300
Artem Bityutskiy <dedekind1@gmail.com> wrote:

> From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
> 
> This patch makes hfsplus stop using the VFS '->write_super()' method along with
> the 's_dirt' superblock flag, because they are on their way out.
> 
> The whole "superblock write-out" VFS infrastructure is served by the
> 'sync_supers()' kernel thread, which wakes up every 5 (by default) seconds and
> writes out all dirty superblocks using the '->write_super()' call-back.  But the
> problem with this thread is that it wastes power by waking up the system every
> 5 seconds, even if there are no diry superblocks, or there are no client
> file-systems which would need this (e.g., btrfs does not use
> '->write_super()'). So we want to kill it completely and thus, we need to make
> file-systems to stop using the '->write_super()' VFS service, and then remove
> it together with the kernel thread.
> 
> Tested using fsstress from the LTP project.
> 
>
> ...
>
> --- a/fs/hfsplus/hfsplus_fs.h
> +++ b/fs/hfsplus/hfsplus_fs.h
> @@ -153,8 +153,11 @@ struct hfsplus_sb_info {
>  	gid_t gid;
>  
>  	int part, session;
> -
>  	unsigned long flags;
> +
> +	int work_queued;               /* non-zero delayed work is queued */

This would be a little nicer if it had the bool type.

> +	struct delayed_work sync_work; /* FS sync delayed work */
> +	spinlock_t work_lock;          /* protects sync_work and work_queued */

I'm not sure that this lock really needs to exist.

> -static void hfsplus_write_super(struct super_block *sb)
> +static void delayed_sync_fs(struct work_struct *work)
>  {
> -	if (!(sb->s_flags & MS_RDONLY))
> -		hfsplus_sync_fs(sb, 1);
> -	else
> -		sb->s_dirt = 0;
> +	struct hfsplus_sb_info *sbi;
> +
> +	sbi = container_of(work, struct hfsplus_sb_info, sync_work.work);
> +
> +	spin_lock(&sbi->work_lock);
> +	sbi->work_queued = 0;
> +	spin_unlock(&sbi->work_lock);

Here it is "protecting" a single write.

> +	hfsplus_sync_fs(sbi->alloc_file->i_sb, 1);
> +}
> +
> +void hfsplus_mark_mdb_dirty(struct super_block *sb)
> +{
> +	struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
> +	unsigned long delay;
> +
> +	if (sb->s_flags & MS_RDONLY)
> +	       return;
> +
> +	spin_lock(&sbi->work_lock);
> +	if (!sbi->work_queued) {
> +	       delay = msecs_to_jiffies(dirty_writeback_interval * 10);
> +	       queue_delayed_work(system_long_wq, &sbi->sync_work, delay);
> +	       sbi->work_queued = 1;
> +	}
> +	spin_unlock(&sbi->work_lock);
>  }

And I think it could be made to go away here, perhaps by switching to
test_and_set_bit or similar.


And I wonder about the queue_delayed_work().  iirc this does nothing to
align timer expiries, so someone who has a lot of filesystems could end
up with *more* timer wakeups.  Shouldn't we do something here to make
the system do larger amounts of work per timer expiry?  Such as the
timer-slack infrastructure?


It strikes me that this whole approach improves the small system with
little write activity, but makes things worse for the larger system
with a lot of filesystems?


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 4/4] hfsplus: get rid of write_super
  2012-06-21 19:41   ` Andrew Morton
@ 2012-06-21 20:30     ` Artem Bityutskiy
  2012-06-21 20:53       ` Artem Bityutskiy
  0 siblings, 1 reply; 10+ messages in thread
From: Artem Bityutskiy @ 2012-06-21 20:30 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Al Viro, Linux FS Maling List, Linux Kernel Maling List

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

On Thu, 2012-06-21 at 12:41 -0700, Andrew Morton wrote:
> > +	spin_lock(&sbi->work_lock);
> > +	if (!sbi->work_queued) {
> > +	       delay = msecs_to_jiffies(dirty_writeback_interval * 10);
> > +	       queue_delayed_work(system_long_wq, &sbi->sync_work, delay);
> > +	       sbi->work_queued = 1;
> > +	}
> > +	spin_unlock(&sbi->work_lock);
> >  }
> 
> And I think it could be made to go away here, perhaps by switching to
> test_and_set_bit or similar.

Yes, you are right, and I thought about this. But I did not want to make
it more complicated than needed. Spinlock is simple and
straight-forward, this is not a fast-path.

> And I wonder about the queue_delayed_work().  iirc this does nothing to
> align timer expiries, so someone who has a lot of filesystems could end
> up with *more* timer wakeups.  Shouldn't we do something here to make
> the system do larger amounts of work per timer expiry?  Such as the
> timer-slack infrastructure?

Well, I also thought about this. But I again did not want to invent
anything complex because main file systems - ext4, btrfs, xfs - do not
use 'write_super()' at all. And then only these dying / rare
file-systems like btrfs / hfs - I did not feel like over-engineering is
needed.

If someone is affected by more timers, which I really really doubt, we
can further optimize this using deferrable timers for some file-systems
which do not really think anything super-important, and we can use range
hrtimers for file-systems which sync something more or less important.
The former is easy to do, the latter would need improving the workqueue
infrastructure.


> It strikes me that this whole approach improves the small system with
> little write activity, but makes things worse for the larger system
> with a lot of filesystems?

Well, if we have a lot of those rare FSes and do not have much
activities, then we do not wake up. If we have a lot of activities,
probably it does not hurt to wake-up much. But of course there is a
situation when this would hurt, but I again, do doubt such systems are
common, an really care.

IOW, I do not dismiss your point, but I think that we rather see if it
affects anyone and then optimize with a deferrable/range timers.

-- 
Best Regards,
Artem Bityutskiy

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 4/4] hfsplus: get rid of write_super
  2012-06-21 20:30     ` Artem Bityutskiy
@ 2012-06-21 20:53       ` Artem Bityutskiy
  0 siblings, 0 replies; 10+ messages in thread
From: Artem Bityutskiy @ 2012-06-21 20:53 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Al Viro, Linux FS Maling List, Linux Kernel Maling List

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

On Thu, 2012-06-21 at 23:30 +0300, Artem Bityutskiy wrote:
> Well, I also thought about this. But I again did not want to invent
> anything complex because main file systems - ext4, btrfs, xfs - do not
> use 'write_super()' at all.

Sorry, ext4 does use it in the non-journal mode but I have patches which
kill it, although not looked at by Ted so far.

>  And then only these dying / rare
> file-systems like btrfs / hfs - I did not feel like over-engineering
> is
> needed.

Sorry, of course I meant reiserfs, not btrfs.

So what's left is hfs/hfsplus, affs, ufs, udf, reiserfs, jffs2. I could
optimize them if needed, but is this worth the effort?

-- 
Best Regards,
Artem Bityutskiy

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 4/4] hfsplus: get rid of write_super
  2012-06-13 11:37 ` [PATCH 4/4] hfsplus: get rid of write_super Artem Bityutskiy
  2012-06-21 19:41   ` Andrew Morton
@ 2012-07-02 14:13   ` Artem Bityutskiy
  1 sibling, 0 replies; 10+ messages in thread
From: Artem Bityutskiy @ 2012-07-02 14:13 UTC (permalink / raw)
  To: Al Viro, Andrew Morton; +Cc: Linux FS Maling List, Linux Kernel Maling List

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

From 6db7ba9f129f1f7bd8c252089700abc6935e3d9a Mon Sep 17 00:00:00 2001
From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Date: Wed, 13 Jun 2012 14:17:40 +0300
Subject: [PATCH 23/23] hfsplus: get rid of write_super

This patch makes hfsplus stop using the VFS '->write_super()' method along with
the 's_dirt' superblock flag, because they are on their way out.

The whole "superblock write-out" VFS infrastructure is served by the
'sync_supers()' kernel thread, which wakes up every 5 (by default) seconds and
writes out all dirty superblocks using the '->write_super()' call-back.  But the
problem with this thread is that it wastes power by waking up the system every
5 seconds, even if there are no diry superblocks, or there are no client
file-systems which would need this (e.g., btrfs does not use
'->write_super()'). So we want to kill it completely and thus, we need to make
file-systems to stop using the '->write_super()' VFS service, and then remove
it together with the kernel thread.

Tested using fsstress from the LTP project.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 fs/hfsplus/bitmap.c     |    4 ++--
 fs/hfsplus/dir.c        |    2 +-
 fs/hfsplus/hfsplus_fs.h |    6 +++++-
 fs/hfsplus/inode.c      |    6 +++---
 fs/hfsplus/super.c      |   41 +++++++++++++++++++++++++++++++++--------
 5 files changed, 44 insertions(+), 15 deletions(-)

|This is patch is the same except that it fixes several white-space
|issues. If you prefer an incremental patch, please, let me know. The
|issues are:
|
|WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (8, 15)
|#170: FILE: fs/hfsplus/super.c:247:
|+       if (sb->s_flags & MS_RDONLY)
|+              return;
|
|WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (8, 15)
|#174: FILE: fs/hfsplus/super.c:251:
|+       if (!sbi->work_queued) {
|+              delay = msecs_to_jiffies(dirty_writeback_interval * 10);

diff --git a/fs/hfsplus/bitmap.c b/fs/hfsplus/bitmap.c
index 1cad80c..4cfbe2e 100644
--- a/fs/hfsplus/bitmap.c
+++ b/fs/hfsplus/bitmap.c
@@ -153,7 +153,7 @@ done:
 	kunmap(page);
 	*max = offset + (curr - pptr) * 32 + i - start;
 	sbi->free_blocks -= *max;
-	sb->s_dirt = 1;
+	hfsplus_mark_mdb_dirty(sb);
 	dprint(DBG_BITMAP, "-> %u,%u\n", start, *max);
 out:
 	mutex_unlock(&sbi->alloc_mutex);
@@ -228,7 +228,7 @@ out:
 	set_page_dirty(page);
 	kunmap(page);
 	sbi->free_blocks += len;
-	sb->s_dirt = 1;
+	hfsplus_mark_mdb_dirty(sb);
 	mutex_unlock(&sbi->alloc_mutex);
 
 	return 0;
diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c
index 26b53fb..8375504 100644
--- a/fs/hfsplus/dir.c
+++ b/fs/hfsplus/dir.c
@@ -316,7 +316,7 @@ static int hfsplus_link(struct dentry *src_dentry, struct inode *dst_dir,
 	inode->i_ctime = CURRENT_TIME_SEC;
 	mark_inode_dirty(inode);
 	sbi->file_count++;
-	dst_dir->i_sb->s_dirt = 1;
+	hfsplus_mark_mdb_dirty(dst_dir->i_sb);
 out:
 	mutex_unlock(&sbi->vh_mutex);
 	return res;
diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
index 66a9365..558dbb4 100644
--- a/fs/hfsplus/hfsplus_fs.h
+++ b/fs/hfsplus/hfsplus_fs.h
@@ -153,8 +153,11 @@ struct hfsplus_sb_info {
 	gid_t gid;
 
 	int part, session;
-
 	unsigned long flags;
+
+	int work_queued;               /* non-zero delayed work is queued */
+	struct delayed_work sync_work; /* FS sync delayed work */
+	spinlock_t work_lock;          /* protects sync_work and work_queued */
 };
 
 #define HFSPLUS_SB_WRITEBACKUP	0
@@ -428,6 +431,7 @@ int hfsplus_show_options(struct seq_file *, struct dentry *);
 
 /* super.c */
 struct inode *hfsplus_iget(struct super_block *, unsigned long);
+void hfsplus_mark_mdb_dirty(struct super_block *sb);
 
 /* tables.c */
 extern u16 hfsplus_case_fold_table[];
diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
index 82b69ee..ad1f16f 100644
--- a/fs/hfsplus/inode.c
+++ b/fs/hfsplus/inode.c
@@ -431,7 +431,7 @@ struct inode *hfsplus_new_inode(struct super_block *sb, umode_t mode)
 		sbi->file_count++;
 	insert_inode_hash(inode);
 	mark_inode_dirty(inode);
-	sb->s_dirt = 1;
+	hfsplus_mark_mdb_dirty(sb);
 
 	return inode;
 }
@@ -442,7 +442,7 @@ void hfsplus_delete_inode(struct inode *inode)
 
 	if (S_ISDIR(inode->i_mode)) {
 		HFSPLUS_SB(sb)->folder_count--;
-		sb->s_dirt = 1;
+		hfsplus_mark_mdb_dirty(sb);
 		return;
 	}
 	HFSPLUS_SB(sb)->file_count--;
@@ -455,7 +455,7 @@ void hfsplus_delete_inode(struct inode *inode)
 		inode->i_size = 0;
 		hfsplus_file_truncate(inode);
 	}
-	sb->s_dirt = 1;
+	hfsplus_mark_mdb_dirty(sb);
 }
 
 void hfsplus_inode_read_fork(struct inode *inode, struct hfsplus_fork_raw *fork)
diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index f4f3d54..cb6b9c1 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -124,7 +124,7 @@ static int hfsplus_system_write_inode(struct inode *inode)
 
 	if (fork->total_size != cpu_to_be64(inode->i_size)) {
 		set_bit(HFSPLUS_SB_WRITEBACKUP, &sbi->flags);
-		inode->i_sb->s_dirt = 1;
+		hfsplus_mark_mdb_dirty(inode->i_sb);
 	}
 	hfsplus_inode_write_fork(inode, fork);
 	if (tree)
@@ -173,7 +173,7 @@ static int hfsplus_sync_fs(struct super_block *sb, int wait)
 
 	dprint(DBG_SUPER, "hfsplus_sync_fs\n");
 
-	sb->s_dirt = 0;
+	cancel_delayed_work(&sbi->sync_work);
 
 	/*
 	 * Explicitly write out the special metadata inodes.
@@ -226,12 +226,34 @@ out:
 	return error;
 }
 
-static void hfsplus_write_super(struct super_block *sb)
+static void delayed_sync_fs(struct work_struct *work)
 {
-	if (!(sb->s_flags & MS_RDONLY))
-		hfsplus_sync_fs(sb, 1);
-	else
-		sb->s_dirt = 0;
+	struct hfsplus_sb_info *sbi;
+
+	sbi = container_of(work, struct hfsplus_sb_info, sync_work.work);
+
+	spin_lock(&sbi->work_lock);
+	sbi->work_queued = 0;
+	spin_unlock(&sbi->work_lock);
+
+	hfsplus_sync_fs(sbi->alloc_file->i_sb, 1);
+}
+
+void hfsplus_mark_mdb_dirty(struct super_block *sb)
+{
+	struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
+	unsigned long delay;
+
+	if (sb->s_flags & MS_RDONLY)
+		return;
+
+	spin_lock(&sbi->work_lock);
+	if (!sbi->work_queued) {
+		delay = msecs_to_jiffies(dirty_writeback_interval * 10);
+		queue_delayed_work(system_long_wq, &sbi->sync_work, delay);
+		sbi->work_queued = 1;
+	}
+	spin_unlock(&sbi->work_lock);
 }
 
 static void hfsplus_put_super(struct super_block *sb)
@@ -240,6 +262,8 @@ static void hfsplus_put_super(struct super_block *sb)
 
 	dprint(DBG_SUPER, "hfsplus_put_super\n");
 
+	cancel_delayed_work_sync(&sbi->sync_work);
+
 	if (!(sb->s_flags & MS_RDONLY) && sbi->s_vhdr) {
 		struct hfsplus_vh *vhdr = sbi->s_vhdr;
 
@@ -325,7 +349,6 @@ static const struct super_operations hfsplus_sops = {
 	.write_inode	= hfsplus_write_inode,
 	.evict_inode	= hfsplus_evict_inode,
 	.put_super	= hfsplus_put_super,
-	.write_super	= hfsplus_write_super,
 	.sync_fs	= hfsplus_sync_fs,
 	.statfs		= hfsplus_statfs,
 	.remount_fs	= hfsplus_remount,
@@ -352,6 +375,8 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent)
 	sb->s_fs_info = sbi;
 	mutex_init(&sbi->alloc_mutex);
 	mutex_init(&sbi->vh_mutex);
+	spin_lock_init(&sbi->work_lock);
+	INIT_DELAYED_WORK(&sbi->sync_work, delayed_sync_fs);
 	hfsplus_fill_defaults(sbi);
 
 	err = -EINVAL;
-- 
1.7.10


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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2012-07-02 14:09 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-06-13 11:37 [PATCH 0/4] hfsplus: stop using write_supers and s_dirt Artem Bityutskiy
2012-06-13 11:37 ` [PATCH 1/4] hfsplus: make hfsplus_sync_fs static Artem Bityutskiy
2012-06-13 11:37 ` [PATCH 2/4] hfsplus: amend debugging print Artem Bityutskiy
2012-06-13 11:37 ` [PATCH 3/4] hfsplus: remove useless check Artem Bityutskiy
2012-06-13 11:37 ` [PATCH 4/4] hfsplus: get rid of write_super Artem Bityutskiy
2012-06-21 19:41   ` Andrew Morton
2012-06-21 20:30     ` Artem Bityutskiy
2012-06-21 20:53       ` Artem Bityutskiy
2012-07-02 14:13   ` Artem Bityutskiy
2012-06-21 12:16 ` [PATCH 0/4] hfsplus: stop using write_supers and s_dirt Artem Bityutskiy

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.