[PATCH v1 0/3] do not use s_dirt in FAT FS

[PATCH v1 0/3] do not use s_dirt in FAT FS

[Artem Bityutskiy]

The FAT file-system uses the VFS '->write_super()' method for writing out
the FSINFO block, which contains useful but not critical information like
the amount of free clusters.

This patch-set makes FAT FS use its own mechanisms for writing-out the FSINFO
block and stop using the '->write_super()' VFS method. Namely, the FAT FS now
submits a delayed work for writing out the FSINFO block once it becomes dirty.

The reason of this exercises is to get rid of the 'sync_supers()' kernel thread.
This kernel thread wakes up every 5 (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, some 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.

Tested with xfstests.

Note: in the past I was trying to upstream patches which optimized 'sync_super()',
but Al Viro wanted me to kill it completely instead, which I am trying to do now.

======
Overall status:

1. ext4: patches submitted, waiting for reply from Ted Ts'o:
   https://lkml.org/lkml/2012/4/2/111
2. ext2: patches are in the ext2 tree maintained by Jan Kara:
   git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git for_next
   However, one patch is still not there:
   http://www.spinics.net/lists/linux-ext4/msg31492.html

TODO: affs, exofs, hfs, hfsplus, jffs2, reiserfs, sysv, udf, ufs
======

Thanks,
Artem.

[PATCH v1 1/3] fat: introduce fat_mark_fsinfo_dirty helper

[Artem Bityutskiy]

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

This is a preparation patch which introduces a 'fat_mark_fsinfo_dirty()'
helper function which just sets the 's_dirt' flag to 1. I'll add more code
to this helper in one of the next patches, and this is why I did not mark
it as 'inline'.

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

diff --git a/fs/fat/fatent.c b/fs/fat/fatent.c
index 2e81ac0..153c053 100644
--- a/fs/fat/fatent.c
+++ b/fs/fat/fatent.c
@@ -308,6 +308,11 @@ void fat_ent_access_init(struct super_block *sb)
 	}
 }
 
+static void fat_mark_fsinfo_dirty(struct super_block *sb)
+{
+	sb->s_dirt = 1;
+}
+
 static inline int fat_ent_update_ptr(struct super_block *sb,
 				     struct fat_entry *fatent,
 				     int offset, sector_t blocknr)
@@ -498,7 +503,7 @@ int fat_alloc_clusters(struct inode *inode, int *cluster, int nr_cluster)
 				sbi->prev_free = entry;
 				if (sbi->free_clusters != -1)
 					sbi->free_clusters--;
-				sb->s_dirt = 1;
+				fat_mark_fsinfo_dirty(sb);
 
 				cluster[idx_clus] = entry;
 				idx_clus++;
@@ -520,7 +525,7 @@ int fat_alloc_clusters(struct inode *inode, int *cluster, int nr_cluster)
 	/* Couldn't allocate the free entries */
 	sbi->free_clusters = 0;
 	sbi->free_clus_valid = 1;
-	sb->s_dirt = 1;
+	fat_mark_fsinfo_dirty(sb);
 	err = -ENOSPC;
 
 out:
@@ -587,7 +592,7 @@ int fat_free_clusters(struct inode *inode, int cluster)
 		ops->ent_put(&fatent, FAT_ENT_FREE);
 		if (sbi->free_clusters != -1) {
 			sbi->free_clusters++;
-			sb->s_dirt = 1;
+			fat_mark_fsinfo_dirty(sb);
 		}
 
 		if (nr_bhs + fatent.nr_bhs > MAX_BUF_PER_PAGE) {
@@ -677,7 +682,7 @@ int fat_count_free_clusters(struct super_block *sb)
 	}
 	sbi->free_clusters = free;
 	sbi->free_clus_valid = 1;
-	sb->s_dirt = 1;
+	fat_mark_fsinfo_dirty(sb);
 	fatent_brelse(&fatent);
 out:
 	unlock_fat(sbi);
-- 
1.7.7.6

[PATCH v1 2/3] fat: mark fsinfo as dirty less often

[Artem Bityutskiy]

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

This is a preparation patch which amends several functions in fatent.c
and makes them call 'fat_mark_fsinfo_dirty()' only once at the end
versus many times in the loop as it is now. Also, this patch makes sure
'fat_mark_fsinfo_dirty()' is called only after we have unlocked the FAT
table ('unlock_fat()').

The reason for this patch is that 'fat_mark_fsinfo_dirty()' will soon
become a bit heavier so it is better to avoid calling it unnecessarily
and while having the FAT table locked.

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

diff --git a/fs/fat/fatent.c b/fs/fat/fatent.c
index 153c053..8e778ef 100644
--- a/fs/fat/fatent.c
+++ b/fs/fat/fatent.c
@@ -503,7 +503,6 @@ int fat_alloc_clusters(struct inode *inode, int *cluster, int nr_cluster)
 				sbi->prev_free = entry;
 				if (sbi->free_clusters != -1)
 					sbi->free_clusters--;
-				fat_mark_fsinfo_dirty(sb);
 
 				cluster[idx_clus] = entry;
 				idx_clus++;
@@ -525,11 +524,11 @@ int fat_alloc_clusters(struct inode *inode, int *cluster, int nr_cluster)
 	/* Couldn't allocate the free entries */
 	sbi->free_clusters = 0;
 	sbi->free_clus_valid = 1;
-	fat_mark_fsinfo_dirty(sb);
 	err = -ENOSPC;
 
 out:
 	unlock_fat(sbi);
+	fat_mark_fsinfo_dirty(sb);
 	fatent_brelse(&fatent);
 	if (!err) {
 		if (inode_needs_sync(inode))
@@ -590,10 +589,8 @@ int fat_free_clusters(struct inode *inode, int cluster)
 		}
 
 		ops->ent_put(&fatent, FAT_ENT_FREE);
-		if (sbi->free_clusters != -1) {
+		if (sbi->free_clusters != -1)
 			sbi->free_clusters++;
-			fat_mark_fsinfo_dirty(sb);
-		}
 
 		if (nr_bhs + fatent.nr_bhs > MAX_BUF_PER_PAGE) {
 			if (sb->s_flags & MS_SYNCHRONOUS) {
@@ -622,6 +619,8 @@ error:
 	for (i = 0; i < nr_bhs; i++)
 		brelse(bhs[i]);
 	unlock_fat(sbi);
+	if (!err)
+		fat_mark_fsinfo_dirty(sb);
 
 	return err;
 }
@@ -682,9 +681,10 @@ int fat_count_free_clusters(struct super_block *sb)
 	}
 	sbi->free_clusters = free;
 	sbi->free_clus_valid = 1;
-	fat_mark_fsinfo_dirty(sb);
 	fatent_brelse(&fatent);
 out:
 	unlock_fat(sbi);
+	if (!err)
+		fat_mark_fsinfo_dirty(sb);
 	return err;
 }
-- 
1.7.7.6

[PATCH v1 3/3] fat: self-manage own fsinfo block

[Artem Bityutskiy]

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

This is the final patch of the series which makes FAT independent of the
VFS '->write_super' method which was used for writing out the FSINFO block
(which contains various non-essential counters like the next free cluster).

Now we just submit a delayed work when the FSINFO block is marked as dirty.
A spinlock is used to make sure we submit only one work at a time. On unmount
or '->sync_fs()', we just flush the workqueue which should make sure the
FSINFO block is written out.

We use the generic 'system_long_wq' for these purposes because it seem to fit
the task well. Indeed, own workqueue would be an overkill and waste of
resources.

The FSINFO write-out delay is the same as before:
'/proc/sys/vm/dirty_expire_centisecs'.

Reminder: this exercises is being done in order to kill the global VFS
'sync_supers()' thread which wakes up the system every 5 seconds no matter
what.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 fs/fat/fat.h    |    7 ++++++-
 fs/fat/fatent.c |   14 +++++++++++++-
 fs/fat/inode.c  |   47 ++++++++++++++++++++++++++++++-----------------
 3 files changed, 49 insertions(+), 19 deletions(-)

diff --git a/fs/fat/fat.h b/fs/fat/fat.h
index 66994f3..4f08ee0 100644
--- a/fs/fat/fat.h
+++ b/fs/fat/fat.h
@@ -7,6 +7,7 @@
 #include <linux/fs.h>
 #include <linux/mutex.h>
 #include <linux/ratelimit.h>
+#include <linux/workqueue.h>
 #include <linux/msdos_fs.h>
 
 /*
@@ -75,7 +76,7 @@ struct msdos_sb_info {
 	struct fat_mount_options options;
 	struct nls_table *nls_disk;  /* Codepage used on disk */
 	struct nls_table *nls_io;    /* Charset used for input and display */
-	const void *dir_ops;		     /* Opaque; default directory operations */
+	const void *dir_ops;         /* Opaque; default directory operations */
 	int dir_per_block;	     /* dir entries per block */
 	int dir_per_block_bits;	     /* log2(dir_per_block) */
 
@@ -87,6 +88,10 @@ struct msdos_sb_info {
 
 	spinlock_t inode_hash_lock;
 	struct hlist_head inode_hashtable[FAT_HASH_SIZE];
+
+	struct delayed_work fsinfo_work; /* FSINFO block write-out work */
+	int fsinfo_dirty; /* non-zero if the FSINFO block should be synced */
+	spinlock_t fsinfo_lock; /* protects fsinfo_* fields */
 };
 
 #define FAT_CACHE_VALID	0	/* special case for valid cache */
diff --git a/fs/fat/fatent.c b/fs/fat/fatent.c
index 8e778ef..8d31994 100644
--- a/fs/fat/fatent.c
+++ b/fs/fat/fatent.c
@@ -310,7 +310,19 @@ void fat_ent_access_init(struct super_block *sb)
 
 static void fat_mark_fsinfo_dirty(struct super_block *sb)
 {
-	sb->s_dirt = 1;
+	struct msdos_sb_info *sbi = MSDOS_SB(sb);
+	unsigned long delay;
+
+	if (sb->s_flags & MS_RDONLY || sbi->fat_bits != 32)
+		return;
+
+	spin_lock(&sbi->fsinfo_lock);
+	if (!sbi->fsinfo_dirty) {
+		delay = msecs_to_jiffies(dirty_writeback_interval * 10);
+		queue_delayed_work(system_long_wq, &sbi->fsinfo_work, delay);
+		sbi->fsinfo_dirty = 1;
+	}
+	spin_unlock(&sbi->fsinfo_lock);
 }
 
 static inline int fat_ent_update_ptr(struct super_block *sb,
diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index 21687e3..ca544b1 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -459,37 +459,49 @@ static void fat_evict_inode(struct inode *inode)
 	fat_detach(inode);
 }
 
-static void fat_write_super(struct super_block *sb)
+static struct msdos_sb_info *work_to_sb(struct work_struct *work)
 {
-	lock_super(sb);
-	sb->s_dirt = 0;
+	struct delayed_work *dwork;
+
+	dwork = container_of(work, struct delayed_work, work);
+	return container_of(dwork, struct msdos_sb_info, fsinfo_work);
+}
+
+static void write_super(struct work_struct *work)
+{
+	struct msdos_sb_info *sbi = work_to_sb(work);
+	struct super_block *sb = sbi->fat_inode->i_sb;
+
+	spin_lock(&sbi->fsinfo_lock);
+	if (!sbi->fsinfo_dirty) {
+		spin_unlock(&sbi->fsinfo_lock);
+		return;
+	}
+	sbi->fsinfo_dirty = 0;
+	spin_unlock(&sbi->fsinfo_lock);
 
-	if (!(sb->s_flags & MS_RDONLY))
-		fat_clusters_flush(sb);
+	lock_super(sb);
+	fat_clusters_flush(sb);
 	unlock_super(sb);
 }
 
 static int fat_sync_fs(struct super_block *sb, int wait)
 {
-	int err = 0;
+	struct msdos_sb_info *sbi = MSDOS_SB(sb);
 
-	if (sb->s_dirt) {
-		lock_super(sb);
-		sb->s_dirt = 0;
-		err = fat_clusters_flush(sb);
-		unlock_super(sb);
-	}
+	if (wait)
+		flush_delayed_work_sync(&sbi->fsinfo_work);
+	else
+		flush_delayed_work(&sbi->fsinfo_work);
 
-	return err;
+	return 0;
 }
 
 static void fat_put_super(struct super_block *sb)
 {
 	struct msdos_sb_info *sbi = MSDOS_SB(sb);
 
-	if (sb->s_dirt)
-		fat_write_super(sb);
-
+	flush_delayed_work_sync(&sbi->fsinfo_work);
 	iput(sbi->fat_inode);
 
 	unload_nls(sbi->nls_disk);
@@ -678,7 +690,6 @@ static const struct super_operations fat_sops = {
 	.write_inode	= fat_write_inode,
 	.evict_inode	= fat_evict_inode,
 	.put_super	= fat_put_super,
-	.write_super	= fat_write_super,
 	.sync_fs	= fat_sync_fs,
 	.statfs		= fat_statfs,
 	.remount_fs	= fat_remount,
@@ -1271,6 +1282,8 @@ int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,
 	sb->s_export_op = &fat_export_ops;
 	ratelimit_state_init(&sbi->ratelimit, DEFAULT_RATELIMIT_INTERVAL,
 			     DEFAULT_RATELIMIT_BURST);
+	INIT_DELAYED_WORK_DEFERRABLE(&sbi->fsinfo_work, write_super);
+	spin_lock_init(&sbi->fsinfo_lock);
 
 	error = parse_options(sb, data, isvfat, silent, &debug, &sbi->options);
 	if (error)
-- 
1.7.7.6

Re: [PATCH v1 0/3] do not use s_dirt in FAT FS

[Andrew Morton]

On Wed, 11 Apr 2012 14:45:04 +0300
Artem Bityutskiy <dedekind1@gmail.com> wrote:

> The FAT file-system uses the VFS '->write_super()' method for writing out
> the FSINFO block, which contains useful but not critical information like
> the amount of free clusters.
> 
> This patch-set makes FAT FS use its own mechanisms for writing-out the FSINFO
> block and stop using the '->write_super()' VFS method. Namely, the FAT FS now
> submits a delayed work for writing out the FSINFO block once it becomes dirty.
> 
> The reason of this exercises is to get rid of the 'sync_supers()' kernel thread.
> This kernel thread wakes up every 5 (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, some 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.
> 
> Tested with xfstests.
> 
> Note: in the past I was trying to upstream patches which optimized 'sync_super()',
> but Al Viro wanted me to kill it completely instead, which I am trying to do now.

hm, I hadn't actually noticed the arrival of the sync_supers kernel
thread.

I don't think that the old scheme of calling sync_supers() via the
writeback (kudate) code was really appropriate.  ->write_super() is
mainly an fs-level function whose role is to push data from in-kernel
data structures down into the buffer_head layer (typically).  That
isn't a writeback function.

> ======
> Overall status:
> 
> 1. ext4: patches submitted, waiting for reply from Ted Ts'o:
>    https://lkml.org/lkml/2012/4/2/111
> 2. ext2: patches are in the ext2 tree maintained by Jan Kara:
>    git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git for_next
>    However, one patch is still not there:
>    http://www.spinics.net/lists/linux-ext4/msg31492.html
> 
> TODO: affs, exofs, hfs, hfsplus, jffs2, reiserfs, sysv, udf, ufs

That's a lot of work.

I don't really like the implementation.  It implies that we'll be
copying and pasting quite a lot of boilerplate delayed-work code into
many different filesystems.  Surely there is a way of hoisting the
common code up into the vfs library layer.

That implies that we retain ->write_super, probably in a modified form.
Modified to permit the VFS to determine whether the superblock needs
treatment, if ->s_dirt doesn't suffice.

The code as you've proposed it will cause more wakeups than needed - if
multiple filesystems are mounted and active, their timers will get out
of sync.  Which rather defeats the intent of the whole work!  This
problem should be addressable via some new centralised way of managing
things.

Re: [PATCH v1 0/3] do not use s_dirt in FAT FS

[Artem Bityutskiy]

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

Hi Andrew, thanks for feed-back! CCing Al.

On Thu, 2012-04-12 at 17:12 -0700, Andrew Morton wrote:
> hm, I hadn't actually noticed the arrival of the sync_supers kernel
> thread.

It arrived when Jens added per-block device BDI threads.

> I don't think that the old scheme of calling sync_supers() via the
> writeback (kudate) code was really appropriate.

Sure.

> > TODO: affs, exofs, hfs, hfsplus, jffs2, reiserfs, sysv, udf, ufs
> 
> That's a lot of work.

Well, may be. But notice:

1. These are baroque file-systems. Modern ones do not use this: xfs,
   btrfs, jfs. Jan Kara has patches which make ext2 stop using
   '->write_super()'. We also sent patches to Ted which make ext4 stop
   using it when it is in non-journal mode (in journal mode it does not
   use it already).

2. Some FSes in the list may not even need '->write_super()'.

Indeed, many file-systems use '->write_super()' to write-out
non-essential optional information, which can be done opportunistically
on unmount/remount/sync_fs instead.

In fact, I think FAT FS is also among those. We do not really need any
timer and it is enough to write out the FSINFO block opportunistically.
Or we may do this in 'fat_write_inode()' when we are writing out the
'fat_inode' which represent the FAT table - seems to make sense because
the FSINFO contains only 2 useful pieces of information about the FAT
table: free clusters and the next free cluster.

I will try this approach.

> I don't really like the implementation.  It implies that we'll be
> copying and pasting quite a lot of boilerplate delayed-work code into
> many different filesystems.  Surely there is a way of hoisting the
> common code up into the vfs library layer.

So basically, my point is:

s/many different filesystems/several baroque file-systems/

> That implies that we retain ->write_super, probably in a modified form.
> Modified to permit the VFS to determine whether the superblock needs
> treatment, if ->s_dirt doesn't suffice.

I tried this approach and it was vetoed by Al Viro. Although it is
simpler to me to resurrect my old patches, I agree with Al that killing
'->write_super()' is a better approach. We do not want to serve a whole
kernel thread in the generic code for few baroque citizens.

Please, see this thread for the reference:
http://lkml.org/lkml/2010/7/22/96

> The code as you've proposed it will cause more wakeups than needed - if
> multiple filesystems are mounted and active, their timers will get out
> of sync.  Which rather defeats the intent of the whole work!  This
> problem should be addressable via some new centralised way of managing
> things.

I do not think this is an issue. If we have many file-systems, and all
of them are actively used so that the super block becomes dirty, which
most probably means there is also write-back - so be it, it is ok to arm
many timers. And if we make them deferrable for most of the FSes (which
we can not do for the generic timer, because we do not know FSes needs)
- then this is not an issue at all.

Also, if you look at this from the angle that only few old FSes will
have this, it becomes not that bad. I assume I will change this
patch-set and won't use delayed works here.

-- 
Best Regards,
Artem Bityutskiy

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

Re: [PATCH v1 0/3] do not use s_dirt in FAT FS

[Andrew Morton]

On Fri, 13 Apr 2012 09:38:28 +0300 Artem Bityutskiy <dedekind1@gmail.com> wrote:

> > That implies that we retain ->write_super, probably in a modified form.
> > Modified to permit the VFS to determine whether the superblock needs
> > treatment, if ->s_dirt doesn't suffice.
> 
> I tried this approach and it was vetoed by Al Viro. Although it is
> simpler to me to resurrect my old patches, I agree with Al that killing
> '->write_super()' is a better approach.

Well, it can be done without a super_operation vector - pass the
library code a superblock* and a function address.  But the difference
is pointless fluff.

> We do not want to serve a whole
> kernel thread in the generic code for few baroque citizens.

One could refcount the thread, but I think I misread the code - the
amount of generic boilerplate which was added to the fs is in fact
pretty small.

> Please, see this thread for the reference:
> http://lkml.org/lkml/2010/7/22/96
> 
> > The code as you've proposed it will cause more wakeups than needed - if
> > multiple filesystems are mounted and active, their timers will get out
> > of sync.  Which rather defeats the intent of the whole work!  This
> > problem should be addressable via some new centralised way of managing
> > things.
> 
> I do not think this is an issue. If we have many file-systems, and all
> of them are actively used so that the super block becomes dirty, which
> most probably means there is also write-back - so be it, it is ok to arm
> many timers. And if we make them deferrable for most of the FSes (which
> we can not do for the generic timer, because we do not know FSes needs)
> - then this is not an issue at all.

OK.

> Also, if you look at this from the angle that only few old FSes will
> have this, it becomes not that bad. I assume I will change this
> patch-set and won't use delayed works here.

I don't think I understand that.  You intend to alter this patchset?

Re: [PATCH v1 0/3] do not use s_dirt in FAT FS

[Artem Bityutskiy]

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

On Fri, 2012-04-13 at 00:26 -0700, Andrew Morton wrote:
> On Fri, 13 Apr 2012 09:38:28 +0300 Artem Bityutskiy <dedekind1@gmail.com> wrote:
> 
> > > That implies that we retain ->write_super, probably in a modified form.
> > > Modified to permit the VFS to determine whether the superblock needs
> > > treatment, if ->s_dirt doesn't suffice.
> > 
> > I tried this approach and it was vetoed by Al Viro. Although it is
> > simpler to me to resurrect my old patches, I agree with Al that killing
> > '->write_super()' is a better approach.
> 
> Well, it can be done without a super_operation vector - pass the
> library code a superblock* and a function address.  But the difference
> is pointless fluff.

May be, let see how many FSes will actually can share things. Per-FS
implementation is better because you do not have to worry about
refcounting and the FS gone by the time a timer expires. Also, when you
know the FS specifics, you can make a decision about whether the timer
can be made deferrable.

Sorry, I did not understand what you meant by "the difference is
pointless fluff" - difference between what and what?

> > Also, if you look at this from the angle that only few old FSes will
> > have this, it becomes not that bad. I assume I will change this
> > patch-set and won't use delayed works here.
> 
> I don't think I understand that.  You intend to alter this patchset?

Yeah, I think I'll be able to implement one of the two ideas I described
in the previous e-mail, test, and send version two of this patch-set.

Thanks!

-- 
Best Regards,
Artem Bityutskiy

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