do not use s_dirt in FAT FS

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.

From: Artem Bityutskiy <[email protected]>

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'.

---
 fs/fat/fatent.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

index 2e81ac0..153c053 100644
@@ -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

From: Artem Bityutskiy <[email protected]>

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.

---
 fs/fat/fatent.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

index 153c053..8e778ef 100644
@@ -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

From: Artem Bityutskiy <[email protected]>

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.

---
 fs/fat/fat.h    |    7 ++++++-
 fs/fat/fatent.c |   14 +++++++++++++-
 fs/fat/inode.c  |   47 ++++++++++++++++++++++++++++++-----------------
 3 files changed, 49 insertions(+), 19 deletions(-)

index 66994f3..4f08ee0 100644
@@ -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 */

index 8e778ef..8d31994 100644
@@ -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,

1.7.7.6

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.

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.

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

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

Sure.

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.

So basically, my point is:

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

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

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

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.

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.

OK.

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

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?

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