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

[Sougata Santra <sougata@tuxera.com>]

On Fri, 2014-07-18 at 12:28 +0400, Vyacheslav Dubeyko wrote:
> 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?

Thanks a lot for taking time to look into the patch. I will look into
it. hfsplus_sync_fs() also called explicitly from mount/unmount (It is
not called from remount, that is a bug and needs to be addressed. ).
This is not required at all since it is already called from vfs. The
only purpose of calling them from mount/unmount is to update dirty/
clear state and other info like driver version, write count etc ...
When clearly hfsplus_sync_fs() does more than updating volume header
and flushing it to disk.

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

Well, it can be done if you think that it is more neat. I have
checked my patch with checkpatch.pl and the tool did not report
any formatting error. I don't know if it reports sub-optimal
label usage. 

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

I think otherwise and I have commented the usage for
hfsplus_sync_volume_header_locked, again if it is considered neat, I
do so.

> 
> > +	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?

Sorry, this is a formatting change and I should not do it. Although,
the line was not tab spaced and doing so exceeded the 80 char limit.

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

Can you please elaborate ? What are the other forks and flags that gets
updated in volume header.

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

No it is called in hfsplus_sync_volume_header().
> 
> 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.

If writing to page-cache is returning error, what is the point of
flushing write back cache of the disk ?.

> 
> >  
> >  	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?

Please see the cover-letter. I think that hfsplus_sync_fs is already
called from vfs.

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

Thanks a lot,

Best regards,
    Sougata.