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

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

[Sougata Santra]

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.

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);
+
+	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);
+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);
+	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);
 		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) {
+		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);
 
 	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);
 	}
 
 	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);
 
 		if (!sbi->hidden_dir) {
 			mutex_lock(&sbi->vh_mutex);

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

[Andrew Morton]

On Thu, 17 Jul 2014 19:32:42 +0300 Sougata Santra <sougata@tuxera.com> 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.

Please fully describe these "problems"?  I'm guessing that the fs will
write the volume header even if it didn't change, so a sync operation
against a completely clean fs still performs a physical write.  But I'd
prefer not to have to guess!

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

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

[Sougata Santra]

On Thu, 2014-07-17 at 13:59 -0700, Andrew Morton wrote:
> On Thu, 17 Jul 2014 19:32:42 +0300 Sougata Santra <sougata@tuxera.com> 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.
> 
> Please fully describe these "problems"?  I'm guessing that the fs will
> write the volume header even if it didn't change, so a sync operation
> against a completely clean fs still performs a physical write.  But I'd
> prefer not to have to guess!

Absolutely sorry, I assumed it was made clear from the cover letter.
Yes, as you described, the problem is that HFS+ syncs volume header
even if it did not change with every sync operation. Also, I have
done some additional changes because I was modifying hfsplus_sync_fs,
and the problems were relevant, and I thought they should be addressed
in the same patch. Please find them below:

Please Note:
-----------
1) hfsplus_sync_volume_header() is added to call from mount/unmount
sequence, since we just want to write the dirty/clean state to disk.
For unmount, hfsplus_sync_fs is already called from sync_filesystem(). 
For mount, it gets called from delayed_sync_fs().

2) Also, there was a error in error propagation. It it also fixed in 
this patch.
-->snip<--
if (!error)
        error2 = error;
-->snap<--

3) The disk is only flushed if there was no error. Previously it was
always flushed without checking the error.

Thanks a lot,

Best regards,
    Sougata
> 
> > 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.

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

[Vyacheslav Dubeyko]

On Fri, 2014-07-18 at 11:35 +0300, Sougata Santra wrote:

[snip]
> 2) Also, there was a error in error propagation. It it also fixed in 
> this patch.
> -->snip<--
> if (!error)
>         error2 = error;
> -->snap<--
> 
> 3) The disk is only flushed if there was no error. Previously it was
> always flushed without checking the error.

So, do you mean that filemap_write_and_wait() and hfsplus_submit_bio()
doesn't request writing on a volume? What do you mean when you are
talking about absence of flush? I think that I have misunderstanding of
the description.

Thanks,
Vyacheslav Dubeyko.

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

[Sougata Santra]

On Fri, 2014-07-18 at 13:01 +0400, Vyacheslav Dubeyko wrote:
> On Fri, 2014-07-18 at 11:35 +0300, Sougata Santra wrote:
> 
> [snip]
> > 2) Also, there was a error in error propagation. It it also fixed in 
> > this patch.
> > -->snip<--
> > if (!error)
> >         error2 = error;
> > -->snap<--
> > 
> > 3) The disk is only flushed if there was no error. Previously it was
> > always flushed without checking the error.
> 
> So, do you mean that filemap_write_and_wait() and hfsplus_submit_bio()
> doesn't request writing on a volume? 
Yes it did, but it wrote in the page-cache ?.
> What do you mean when you are
> talking about absence of flush? I think that I have misunderstanding of
> the description.
AFAIK, blkdev_issue_flush() is issued to flush the write back cache of 
the block device if it supports REQUEST_FLUSH. I did not understand the
need to flush the disk cache to send everything into non-volatile
memory when writing to page-cache returned some error. If the error
checking is not required, then I can remove it.

> 
> Thanks,
> Vyacheslav Dubeyko.
> 
> 

Thanks a lot,
    Sougata.

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

[Vyacheslav Dubeyko]

On Jul 18, 2014, at 1:49 PM, Sougata Santra wrote:

> On Fri, 2014-07-18 at 13:01 +0400, Vyacheslav Dubeyko wrote:
>> On Fri, 2014-07-18 at 11:35 +0300, Sougata Santra wrote:
>> 
>> [snip]
>>> 2) Also, there was a error in error propagation. It it also fixed in 
>>> this patch.
>>> -->snip<--
>>> if (!error)
>>>        error2 = error;
>>> -->snap<--
>>> 
>>> 3) The disk is only flushed if there was no error. Previously it was
>>> always flushed without checking the error.
>> 
>> So, do you mean that filemap_write_and_wait() and hfsplus_submit_bio()
>> doesn't request writing on a volume? 
> Yes it did, but it wrote in the page-cache ?.

The hfsplus_submit_bio() makes bio allocation via bio_alloc() and, then,
send request on write into elevator queue by means of submit_bio_wait().
Moreover, submit_bio_wait() submits a bio, and wait until it completes.
It means that we have changed block on volume after returning from
hfsplus_submit_bio().

The filemap_write_and_wait() calls do_writepages() finally. It calls
mapping->a_ops->writepages(). So, it means that do_writepages() calls
hfsplus_writepages(). As a result, finally, requests on write are placed in
elevator queue. And, again, dirty pages of inode->i_mapping (page cache)
will be on volume after returning from filemap_write_and_wait().

This is my understanding of hfsplus_sync_fs() method's logic. Please,
correct me if I am wrong.

So, if you try to prevent from blkdev_issue_flush() then it doesn't mean
that you prevent from writing special files and volume header on volume.
I suppose that logic of writing in any case was special policy. Because
fsck utility can help to us in some bad situations. This is my understanding
of this method's logic.

Thanks,
Vyacheslav Dubeyko. 

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

[sougata santra]

On 07/19/2014 01:58 PM, Vyacheslav Dubeyko wrote:
>
> On Jul 18, 2014, at 1:49 PM, Sougata Santra wrote:
>
>> On Fri, 2014-07-18 at 13:01 +0400, Vyacheslav Dubeyko wrote:
>>> On Fri, 2014-07-18 at 11:35 +0300, Sougata Santra wrote:
>>>
>>> [snip]
>>>> 2) Also, there was a error in error propagation. It it also fixed in
>>>> this patch.
>>>> -->snip<--
>>>> if (!error)
>>>>         error2 = error;
>>>> -->snap<--
>>>>
>>>> 3) The disk is only flushed if there was no error. Previously it was
>>>> always flushed without checking the error.
>>>
>>> So, do you mean that filemap_write_and_wait() and hfsplus_submit_bio()
>>> doesn't request writing on a volume?
>> Yes it did, but it wrote in the page-cache ?.
>
> The hfsplus_submit_bio() makes bio allocation via bio_alloc() and, then,
> send request on write into elevator queue by means of submit_bio_wait().
> Moreover, submit_bio_wait() submits a bio, and wait until it completes.
> It means that we have changed block on volume after returning from
> hfsplus_submit_bio().

AFAIK, this is not always correct, we don't change the block on the
volume. The block device driver uses special command to initiate data
transfer with the disk controller. Then the disk controller uses DMA to
transfer the data and raises an interrupt that it is done with the data
transfer. That does not mean that the data is there is in the
non-volatile memory of the disk. If there is a write back cache in the
disk then the data is stored there. Then the disk controller uses its
own algorithm to write back data from its own cache to disk. The flush
operation sends an empty bio requesting flush of the write back cache.
My argument was when there was a problem of transferring the data into
disk cache, why bother to flush it. And if any data has already been
written to disk cache, the disk controller takes care and writes it
into non volatile memory, given that there is no power-off/plug-out
scenarios.

>
> The filemap_write_and_wait() calls do_writepages() finally. It calls
> mapping->a_ops->writepages(). So, it means that do_writepages() calls
> hfsplus_writepages(). As a result, finally, requests on write are placed in
> elevator queue. And, again, dirty pages of inode->i_mapping (page cache)
> will be on volume after returning from filemap_write_and_wait().
>
> This is my understanding of hfsplus_sync_fs() method's logic. Please,
> correct me if I am wrong.
>
> So, if you try to prevent from blkdev_issue_flush() then it doesn't mean
> that you prevent from writing special files and volume header on volume.
> I suppose that logic of writing in any case was special policy. Because
> fsck utility can help to us in some bad situations. This is my understanding
> of this method's logic.

Please don't get incorrect. I am not sure if should be done or not.
Obviously if it is not the best thing to do, we won't do it :-).

>
> Thanks,
> Vyacheslav Dubeyko.
>

Thanks a lot,
   Sougata Santra

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

[Vyacheslav Dubeyko]

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.

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

[Sougata Santra]

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.

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

[Vyacheslav Dubeyko]

On Jul 18, 2014, at 1:24 PM, Sougata Santra wrote:

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

Finally, I think that more compact and better solution can be achieved by
modification of hfsplus_mark_mdb_dirty() using.

We can add into struct hfsplus_sb_info two additional fields:
(1) flag that informs about dirty state of volume header;
(2) last write timestamp.

So, every calling of hfsplus_mark_mdb_dirty() will set flag of volume header
dirtiness. The hfsplus_sync_fs() will know that volume header is really dirty
on basis of checking dirty flag in hfsplus_sb_info. And hfsplus_sync_fs() will
clear this flag after saving state of volume header and special files on volume.

The last write timestamp + timeout between two adjacent writes can be used
for decreasing frequency of flushing of volume header on volume. Such technique
is used in NILFS2.

[snip]
>> 
>>> 		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.
> 

There is simple use-case. For example, you mount file system, do nothing
with it and, finally, unmount file system. You should set
kHFSBootVolumeInconsistentBit in attributes field of volume header during
mount. So, you should write new state of volume header on volume.
Then, during unmount, you should set kHFSVolumeUnmountedBit in
attributes field of volume header and save this state of volume header on
volume. This flags are used by fsck and file system driver for detecting
presence of error on volume.  

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

[snip]
>> 
>>> 
>>> 	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.

Anyway, I prefer to leave code here unchanged. It is really important to guarantee
that we will have file system in consistent way after unmount. If special files were
flushed previously then it will done nothing here for special files.

Thanks,
Vyacheslav Dubeyko.

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

[sougata santra]

On 07/19/2014 02:23 PM, Vyacheslav Dubeyko wrote:
>
> On Jul 18, 2014, at 1:24 PM, Sougata Santra wrote:
>
>> 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.
>>
>
> Finally, I think that more compact and better solution can be achieved by
> modification of hfsplus_mark_mdb_dirty() using.
>
> We can add into struct hfsplus_sb_info two additional fields:
> (1) flag that informs about dirty state of volume header;
> (2) last write timestamp.
>
> So, every calling of hfsplus_mark_mdb_dirty() will set flag of volume header
> dirtiness. The hfsplus_sync_fs() will know that volume header is really dirty
> on basis of checking dirty flag in hfsplus_sb_info. And hfsplus_sync_fs() will
> clear this flag after saving state of volume header and special files on volume.
>
> The last write timestamp + timeout between two adjacent writes can be used
> for decreasing frequency of flushing of volume header on volume. Such technique
> is used in NILFS2.

Thank you for the pointer, will look into it. I guess it has to be
locked against mutual access as well.

>
> [snip]
>>>
>>>> 		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.
>>
>
> There is simple use-case. For example, you mount file system, do nothing
> with it and, finally, unmount file system. You should set
> kHFSBootVolumeInconsistentBit in attributes field of volume header during
> mount. So, you should write new state of volume header on volume.
> Then, during unmount, you should set kHFSVolumeUnmountedBit in
> attributes field of volume header and save this state of volume header on
> volume. This flags are used by fsck and file system driver for detecting
> presence of error on volume.
>
Please look at the patch, it separates out the mount/unmount code flow
from sync file system code flow. Volume header is modified by both when
syncing file-system (because changes in allocations..etc), and during
mount and unmount. These are separate context and should not be mixed
with each other, please see any other file-system. When volume
dirty/clean states are updated, other version info etc are updated
they should be immediately flushed to disk that is it, but sync file
system context is much more than that.

AFAIK the relevant checks in sync_fs does some check to see if any
allocation states have changed, I agree this can be done in
hfsplus_mark_mdb_dirty but I need to check that. Although the
necessary checks that are done are only checking if the volume
header needs to be updated and to the best of my knowledge it
checks all the relevant fields which can get modified from
hfsplus_sync_fs() context (not mount/unmount context).

>>> 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.
>>
>
> [snip]
>>>
>>>>
>>>> 	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.
>
> Anyway, I prefer to leave code here unchanged. It is really important to guarantee
> that we will have file system in consistent way after unmount. If special files were
> flushed previously then it will done nothing here for special files.

I don't really understand.

>
> Thanks,
> Vyacheslav Dubeyko.
>

Thanks
     Sougata.