[PATCH] FAT: optimize FSINFO writeback

[PATCH] FAT: optimize FSINFO writeback

[steve]

Only write the FSINFO block back to disk when its contents change.
This optimization can be important when the underlying physical media
can wear out, i.e. Flash.

Signed-off-by: Steven J. Magnani <steve@digidescorp.com>
---
diff -uprN a/fs/fat/misc.c b/fs/fat/misc.c
--- a/fs/fat/misc.c	2009-06-29 11:12:40.000000000 -0500
+++ b/fs/fat/misc.c	2009-06-29 11:46:45.000000000 -0500
@@ -61,11 +61,25 @@ void fat_clusters_flush(struct super_blo
 		       le32_to_cpu(fsinfo->signature2),
 		       sbi->fsinfo_sector);
 	} else {
-		if (sbi->free_clusters != -1)
-			fsinfo->free_clusters = cpu_to_le32(sbi->free_clusters);
-		if (sbi->prev_free != -1)
-			fsinfo->next_cluster = cpu_to_le32(sbi->prev_free);
-		mark_buffer_dirty(bh);
+		char write_needed = 0;
+		__le32 le_value;
+
+		if (sbi->free_clusters != -1) {
+			le_value = cpu_to_le32(sbi->free_clusters);
+			if (fsinfo->free_clusters != le_value) {
+				fsinfo->free_clusters = le_value;
+				write_needed = 1;
+			}
+		}
+		if (sbi->prev_free != -1) {
+			le_value = cpu_to_le32(sbi->prev_free);
+			if (fsinfo->next_cluster != le_value) {
+				fsinfo->next_cluster = le_value;
+				write_needed = 1;
+			}
+		}
+		if (write_needed)
+			mark_buffer_dirty(bh);
 	}
 	brelse(bh);
 }

Re: [PATCH] FAT: optimize FSINFO writeback

[OGAWA Hirofumi]

steve@digidescorp.com writes:

> Only write the FSINFO block back to disk when its contents change.
> This optimization can be important when the underlying physical media
> can wear out, i.e. Flash.

I have no objection to this though.  Was this tested on recent version?
Well, now, we are using sb->s_dirty for fsinfo, so I'm wondering why
this happen frequently.

Thanks.

> Signed-off-by: Steven J. Magnani <steve@digidescorp.com>
> ---
> diff -uprN a/fs/fat/misc.c b/fs/fat/misc.c
> --- a/fs/fat/misc.c	2009-06-29 11:12:40.000000000 -0500
> +++ b/fs/fat/misc.c	2009-06-29 11:46:45.000000000 -0500
> @@ -61,11 +61,25 @@ void fat_clusters_flush(struct super_blo
>  		       le32_to_cpu(fsinfo->signature2),
>  		       sbi->fsinfo_sector);
>  	} else {
> -		if (sbi->free_clusters != -1)
> -			fsinfo->free_clusters = cpu_to_le32(sbi->free_clusters);
> -		if (sbi->prev_free != -1)
> -			fsinfo->next_cluster = cpu_to_le32(sbi->prev_free);
> -		mark_buffer_dirty(bh);
> +		char write_needed = 0;
> +		__le32 le_value;
> +
> +		if (sbi->free_clusters != -1) {
> +			le_value = cpu_to_le32(sbi->free_clusters);
> +			if (fsinfo->free_clusters != le_value) {
> +				fsinfo->free_clusters = le_value;
> +				write_needed = 1;
> +			}
> +		}
> +		if (sbi->prev_free != -1) {
> +			le_value = cpu_to_le32(sbi->prev_free);
> +			if (fsinfo->next_cluster != le_value) {
> +				fsinfo->next_cluster = le_value;
> +				write_needed = 1;
> +			}
> +		}
> +		if (write_needed)
> +			mark_buffer_dirty(bh);
>  	}
>  	brelse(bh);
>  }
>

-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] FAT: optimize FSINFO writeback

[Steven J. Magnani]

On Wed, 2009-07-01 at 05:57 +0900, OGAWA Hirofumi wrote:
> steve@digidescorp.com writes:
> 
> > Only write the FSINFO block back to disk when its contents change.
> > This optimization can be important when the underlying physical media
> > can wear out, i.e. Flash.
> 
> I have no objection to this though.  Was this tested on recent version?

I tested it on 2.6.30.

> Well, now, we are using sb->s_dirty for fsinfo, so I'm wondering why
> this happen frequently.

My scenario was modifying a sector of an existing file and using
fdatasync() to flush it. The FSINFO sector was being updated even though
nothing about the FAT layout had changed.

> 
> Thanks.

Regards,
------------------------------------------------------------------------
 Steven J. Magnani               "I claim this network for MARS!
 www.digidescorp.com              Earthling, return my space modulator!"

 #include <standard.disclaimer>

Re: [PATCH] FAT: optimize FSINFO writeback

[OGAWA Hirofumi]

"Steven J. Magnani" <steve@digidescorp.com> writes:

>> Well, now, we are using sb->s_dirty for fsinfo, so I'm wondering why
>> this happen frequently.
>
> My scenario was modifying a sector of an existing file and using
> fdatasync() to flush it. The FSINFO sector was being updated even though
> nothing about the FAT layout had changed.

I see. Probably, I'm missing something, or handling of sb->s_dirt may be
buggy, or something.

If it was fixed, is this patch still needed?  I guess this patch would
still be useful on some case though.  If you can explain, it would be
good.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] FAT: optimize FSINFO writeback

[Steven J. Magnani]

On Wed, 2009-07-01 at 09:28 +0900, OGAWA Hirofumi wrote:
> "Steven J. Magnani" <steve@digidescorp.com> writes:
> 
> >> Well, now, we are using sb->s_dirty for fsinfo, so I'm wondering why
> >> this happen frequently.
> >
> > My scenario was modifying a sector of an existing file and using
> > fdatasync() to flush it. The FSINFO sector was being updated even though
> > nothing about the FAT layout had changed.
> 
> I see. Probably, I'm missing something, or handling of sb->s_dirt may be
> buggy, or something.
> 
> If it was fixed, is this patch still needed?  I guess this patch would
> still be useful on some case though.  If you can explain, it would be
> good.

It is I who am missing something. The patch originated against a 2.6.20
kernel, where it does indeed suppress unnecessary updates. I saw the
same code in the 2.6.30 kernel and assumed the same issue was present,
and tested that with the patch present there were no unnecessary
updates. It appears that there are no unnecessary updates even _without_
the patch, so I withdraw it.

> 
> Thanks.

Thanks for looking at this so carefully.

Regards,
------------------------------------------------------------------------
 Steven J. Magnani               "I claim this network for MARS!
 www.digidescorp.com              Earthling, return my space modulator!"

 #include <standard.disclaimer>