Re: [PATCH 1/1] hfsplus: skip unnecessary volume header sync

[Vyacheslav Dubeyko <slava@dubeyko.com>]

On Thu, 2014-07-17 at 19:32 +0300, Sougata Santra wrote:
> hfsplus_sync_fs always updates volume header information to disk with every
> sync. This causes problem for systems trying to monitor disk activity to
> switch them to low power state. Also hfsplus_sync_fs is explicitly called
> from mount/unmount, which is unnecessary. During mount/unmount we only want
> to set/clear volume dirty sate.
> 

As far as I can judge, hfsplus driver has hfsplus_mark_mdb_dirty()
method. This method "marks" volume header as dirty and to define some
dirty_writeback_interval. The hfsplus_mark_mdb_dirty() is called in such
important methods as hfsplus_block_allocate(), hfsplus_block_free(),
hfsplus_system_write_inode(), hfsplus_link(), hfsplus_new_inode(),
hfsplus_delete_inode(). So, it means for me that every call of
hfsplus_sync_fs() is made when volume header should be written on
volume. So, if you can detect some inefficiency or frequent calls of
hfsplus_sync_fs() then, maybe, it needs to optimize
hfsplus_mark_mdb_dirty() in the direction of proper
dirty_writeback_interval definition. What do you think?

> Signed-off-by: Sougata Santra <sougata@tuxera.com>
> ---
>  fs/hfsplus/super.c | 101 +++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 79 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
> index 4cf2024..5cacb06 100644
> --- a/fs/hfsplus/super.c
> +++ b/fs/hfsplus/super.c
> @@ -170,12 +170,61 @@ static void hfsplus_evict_inode(struct inode *inode)
>  	}
>  }
>  
> +/*
> + * Helper to sync volume header state to disk. Caller must acquire
> + * volume header mutex (vh_mutex).
> + */
> +static int hfsplus_sync_volume_header_locked(struct super_block *sb)
> +{
> +	struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
> +	int write_backup = 0;
> +	int error = 0;
> +
> +	if (test_and_clear_bit(HFSPLUS_SB_WRITEBACKUP, &sbi->flags)) {
> +		memcpy(sbi->s_backup_vhdr, sbi->s_vhdr, sizeof(*sbi->s_vhdr));
> +		write_backup = 1;
> +	}
> +
> +	error = hfsplus_submit_bio(sb,
> +			sbi->part_start + HFSPLUS_VOLHEAD_SECTOR,
> +			sbi->s_vhdr_buf, NULL, WRITE_SYNC);

Such formatting looks weird for me. Maybe, it makes sense to use local
variables here?

> +
> +	if (error || !write_backup)
> +		goto out;
> +
> +	error = hfsplus_submit_bio(sb,
> +			sbi->part_start + sbi->sect_count - 2,
> +			sbi->s_backup_vhdr_buf, NULL, WRITE_SYNC);

Ditto.

> +out:
> +	return error;
> +}
> +
> +/* Sync dirty/clean volume header state to disk. */
> +static int hfsplus_sync_volume_header(struct super_block *sb)
> +{
> +	struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
> +	int error = 0;
> +
> +	hfs_dbg(SUPER, "hfsplus_sync_volume_header\n");
> +
> +	mutex_lock(&sbi->vh_mutex);
> +	error = hfsplus_sync_volume_header_locked(sb);

Name as hfsplus_sync_volume_header_locked() really confuses me. Because
it is possible to think that we've locked mutex inside method. So, I
suppose that hfsplus_sync_volume_header_unlocked() is much better name
for my taste.

> +	mutex_unlock(&sbi->vh_mutex);
> +
> +	if (!error && !test_bit(HFSPLUS_SB_NOBARRIER, &sbi->flags))
> +		blkdev_issue_flush(sb->s_bdev, GFP_KERNEL, NULL);
> +
> +	return error;
> +}
> +
>  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;
> -	int write_backup = 0;
> +	int write_header = 0;
>  	int error, error2;
> +	u32 free_blocks, next_cnid;
> +	u32 folder_count, file_count;
>  
>  	if (!wait)
>  		return 0;
> @@ -196,7 +245,8 @@ static int hfsplus_sync_fs(struct super_block *sb, int wait)
>  		error = error2;
>  	if (sbi->attr_tree) {
>  		error2 =
> -		    filemap_write_and_wait(sbi->attr_tree->inode->i_mapping);
> +			filemap_write_and_wait(
> +					sbi->attr_tree->inode->i_mapping);

What purpose has such change?

>  		if (!error)
>  			error = error2;
>  	}
> @@ -206,34 +256,41 @@ static int hfsplus_sync_fs(struct super_block *sb, int wait)
>  
>  	mutex_lock(&sbi->vh_mutex);
>  	mutex_lock(&sbi->alloc_mutex);
> -	vhdr->free_blocks = cpu_to_be32(sbi->free_blocks);
> -	vhdr->next_cnid = cpu_to_be32(sbi->next_cnid);
> -	vhdr->folder_count = cpu_to_be32(sbi->folder_count);
> -	vhdr->file_count = cpu_to_be32(sbi->file_count);
>  
> -	if (test_and_clear_bit(HFSPLUS_SB_WRITEBACKUP, &sbi->flags)) {
> -		memcpy(sbi->s_backup_vhdr, sbi->s_vhdr, sizeof(*sbi->s_vhdr));
> -		write_backup = 1;
> +	free_blocks = cpu_to_be32(sbi->free_blocks);
> +	next_cnid = cpu_to_be32(sbi->next_cnid);
> +	folder_count = cpu_to_be32(sbi->folder_count);
> +	file_count = cpu_to_be32(sbi->file_count);
> +
> +	/* Check if some attribute of volume header has changed. */
> +	if (vhdr->free_blocks != free_blocks ||
> +			vhdr->next_cnid != next_cnid ||
> +			vhdr->folder_count != folder_count ||
> +			vhdr->file_count != file_count) {

I don't think that this check is correct because volume header contains
some flags and forks. Moreover, there is specially dedicated method for
"marking" volume header as dirty (I mean hfsplus_mark_mdb_dirty()
method). So, I don't think that this check is really necessary. And,
moreover, I don't think such significant modification of
hfsplus_sync_fs() makes sense at all.

> +		vhdr->free_blocks = free_blocks;
> +		vhdr->next_cnid = next_cnid;
> +		vhdr->folder_count = folder_count;
> +		vhdr->file_count = file_count;
> +		write_header = 1;
>  	}
> +	/*
> +	 * Write volume header only when something has changed. Also there
> +	 * is no need to write backup volume header if nothing has changed
> +	 * in the the volume header itself.
> +	 */
> +	if (!write_header)
> +		goto out;
>  
> -	error2 = hfsplus_submit_bio(sb,
> -				   sbi->part_start + HFSPLUS_VOLHEAD_SECTOR,
> -				   sbi->s_vhdr_buf, NULL, WRITE_SYNC);
> +	error2 = hfsplus_sync_volume_header_locked(sb);
>  	if (!error)
>  		error = error2;
> -	if (!write_backup)
> -		goto out;
>  
> -	error2 = hfsplus_submit_bio(sb,
> -				  sbi->part_start + sbi->sect_count - 2,
> -				  sbi->s_backup_vhdr_buf, NULL, WRITE_SYNC);
> -	if (!error)
> -		error2 = error;
>  out:
>  	mutex_unlock(&sbi->alloc_mutex);
>  	mutex_unlock(&sbi->vh_mutex);
>  
> -	if (!test_bit(HFSPLUS_SB_NOBARRIER, &sbi->flags))
> +	if (write_header && !error &&
> +			!test_bit(HFSPLUS_SB_NOBARRIER, &sbi->flags))
>  		blkdev_issue_flush(sb->s_bdev, GFP_KERNEL, NULL);

The blkdev_issue_flush() is called in
hfsplus_sync_volume_header_locked() yet.

Do you really confident that it makes sense to prevent from calling
blkdev_issue_flush() here in the case of error detection? Frankly
speaking, I doubt that it makes sense.

>  
>  	return error;
> @@ -287,7 +344,7 @@ static void hfsplus_put_super(struct super_block *sb)
>  		vhdr->attributes |= cpu_to_be32(HFSPLUS_VOL_UNMNT);
>  		vhdr->attributes &= cpu_to_be32(~HFSPLUS_VOL_INCNSTNT);
>  
> -		hfsplus_sync_fs(sb, 1);
> +		hfsplus_sync_volume_header(sb);

I doubt that to flush the volume header only is proper approach. Could
you guarantee that special metadata files have been flushed before?

>  	}
>  
>  	hfs_btree_close(sbi->attr_tree);
> @@ -539,7 +596,7 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent)
>  		be32_add_cpu(&vhdr->write_count, 1);
>  		vhdr->attributes &= cpu_to_be32(~HFSPLUS_VOL_UNMNT);
>  		vhdr->attributes |= cpu_to_be32(HFSPLUS_VOL_INCNSTNT);
> -		hfsplus_sync_fs(sb, 1);
> +		hfsplus_sync_volume_header(sb);

Yes, maybe, it makes sense to flush the volume header only here.

Thanks,
Vyacheslav Dubeyko.