hfsplus patch review

hfsplus patch review

[Christoph Hellwig]

Here's my current hfsplus patch queue for review.  The biggest bits
are some major fsync and sync surgery, making sure we actually write
data all the way to the disk when we're supposed to it.  In addition
to that tone down some printks and get rid of the buffer_head usage
to actually allow us to access the backup volume header in all cases.

[PATCH 1/11] hfsplus: silence a few debug printks

[Christoph Hellwig]

Turn a few noisy debug printks that show up during xfstests into
complied out debug print statements.

Signed-off-by: Christoph Hellwig <hch@tuxera.com>

Index: linux-2.6/fs/hfsplus/bnode.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/bnode.c	2010-11-17 22:43:17.971254227 +0100
+++ linux-2.6/fs/hfsplus/bnode.c	2010-11-17 22:43:21.006253879 +0100
@@ -358,7 +358,7 @@ void hfs_bnode_unlink(struct hfs_bnode *
 
 	// move down?
 	if (!node->prev && !node->next) {
-		printk(KERN_DEBUG "hfs_btree_del_level\n");
+		dprint(DBG_BNODE_MOD, "hfs_btree_del_level\n");
 	}
 	if (!node->parent) {
 		tree->root = 0;
Index: linux-2.6/fs/hfsplus/brec.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/brec.c	2010-11-17 22:43:17.978254437 +0100
+++ linux-2.6/fs/hfsplus/brec.c	2010-11-17 22:43:21.061253809 +0100
@@ -375,7 +375,7 @@ again:
 		end_off = hfs_bnode_read_u16(parent, end_rec_off);
 		if (end_rec_off - end_off < diff) {
 
-			printk(KERN_DEBUG "hfs: splitting index node...\n");
+			dprint(DBG_BNODE_MOD, "hfs: splitting index node.\n");
 			fd->bnode = parent;
 			new_node = hfs_bnode_split(fd);
 			if (IS_ERR(new_node))
Index: linux-2.6/fs/hfsplus/btree.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/btree.c	2010-11-17 22:43:17.992253948 +0100
+++ linux-2.6/fs/hfsplus/btree.c	2010-11-17 22:43:21.226254856 +0100
@@ -287,7 +287,7 @@ struct hfs_bnode *hfs_bmap_alloc(struct
 		kunmap(*pagep);
 		nidx = node->next;
 		if (!nidx) {
-			printk(KERN_DEBUG "hfs: create new bmap node...\n");
+			dprint(DBG_BNODE_MOD, "hfs: create new bmap node.\n");
 			next_node = hfs_bmap_new_bmap(node, idx);
 		} else
 			next_node = hfs_bnode_find(tree, nidx);
Index: linux-2.6/fs/hfsplus/super.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/super.c	2010-11-17 22:43:18.001254437 +0100
+++ linux-2.6/fs/hfsplus/super.c	2010-11-17 22:43:21.231254298 +0100
@@ -451,8 +451,6 @@ static int hfsplus_fill_super(struct sup
 	sync_dirty_buffer(sbi->s_vhbh);
 
 	if (!sbi->hidden_dir) {
-		printk(KERN_DEBUG "hfs: create hidden dir...\n");
-
 		mutex_lock(&sbi->vh_mutex);
 		sbi->hidden_dir = hfsplus_new_inode(sb, S_IFDIR);
 		hfsplus_create_cat(sbi->hidden_dir->i_ino, sb->s_root->d_inode,

[PATCH 2/11] hfsplus: always use hfsplus_sync_fs to write the volume header

[Christoph Hellwig]

Remove opencoded writing of the volume header in hfsplus_fill_super
and hfsplus_put_super and offload it to hfsplus_sync_fs.  In the
put_super case this means we only write the superblock once instead
of twice.

Signed-off-by: Christoph Hellwig <hch@tuxera.com>

Index: linux-2.6/fs/hfsplus/super.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/super.c	2010-11-17 22:43:21.231254298 +0100
+++ linux-2.6/fs/hfsplus/super.c	2010-11-17 22:43:25.022004054 +0100
@@ -215,16 +215,14 @@ static void hfsplus_put_super(struct sup
 	if (!sb->s_fs_info)
 		return;
 
-	if (sb->s_dirt)
-		hfsplus_write_super(sb);
 	if (!(sb->s_flags & MS_RDONLY) && sbi->s_vhdr) {
 		struct hfsplus_vh *vhdr = sbi->s_vhdr;
 
 		vhdr->modify_date = hfsp_now2mt();
 		vhdr->attributes |= cpu_to_be32(HFSPLUS_VOL_UNMNT);
 		vhdr->attributes &= cpu_to_be32(~HFSPLUS_VOL_INCNSTNT);
-		mark_buffer_dirty(sbi->s_vhbh);
-		sync_dirty_buffer(sbi->s_vhbh);
+
+		hfsplus_sync_fs(sb, 1);
 	}
 
 	hfs_btree_close(sbi->cat_tree);
@@ -447,8 +445,7 @@ static int hfsplus_fill_super(struct sup
 	be32_add_cpu(&vhdr->write_count, 1);
 	vhdr->attributes &= cpu_to_be32(~HFSPLUS_VOL_UNMNT);
 	vhdr->attributes |= cpu_to_be32(HFSPLUS_VOL_INCNSTNT);
-	mark_buffer_dirty(sbi->s_vhbh);
-	sync_dirty_buffer(sbi->s_vhbh);
+	hfsplus_sync_fs(sb, 1);
 
 	if (!sbi->hidden_dir) {
 		mutex_lock(&sbi->vh_mutex);

[PATCH 3/11] hfsplus: use raw bio access for the volume headers

[Christoph Hellwig]

The hfsplus backup volume header is located two blocks from the end of
the device.  In case of device sizes that are not 4k aligned this means
we can't access it using buffer_heads when using the default 4k block
size.

Switch to using raw bios to read/write all buffer headers.  We were not
relying on any caching behaviour of the buffer heads anyway.  Additionally
always read in the backup volume header during mount to verify that we
can actually read it.

Signed-off-by: Christoph Hellwig <hch@tuxera.com>

Index: linux-2.6/fs/hfsplus/super.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/super.c	2010-11-17 22:45:16.985254227 +0100
+++ linux-2.6/fs/hfsplus/super.c	2010-11-17 22:46:55.619254646 +0100
@@ -157,45 +157,40 @@ int hfsplus_sync_fs(struct super_block *
 {
 	struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
 	struct hfsplus_vh *vhdr = sbi->s_vhdr;
+	int write_backup = 0;
+	int error, error2;
 
 	dprint(DBG_SUPER, "hfsplus_write_super\n");
 
-	mutex_lock(&sbi->vh_mutex);
-	mutex_lock(&sbi->alloc_mutex);
 	sb->s_dirt = 0;
 
+	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);
 
-	mark_buffer_dirty(sbi->s_vhbh);
 	if (test_and_clear_bit(HFSPLUS_SB_WRITEBACKUP, &sbi->flags)) {
-		if (sbi->sect_count) {
-			struct buffer_head *bh;
-			u32 block, offset;
-
-			block = sbi->blockoffset;
-			block += (sbi->sect_count - 2) >> (sb->s_blocksize_bits - 9);
-			offset = ((sbi->sect_count - 2) << 9) & (sb->s_blocksize - 1);
-			printk(KERN_DEBUG "hfs: backup: %u,%u,%u,%u\n",
-					  sbi->blockoffset, sbi->sect_count,
-					  block, offset);
-			bh = sb_bread(sb, block);
-			if (bh) {
-				vhdr = (struct hfsplus_vh *)(bh->b_data + offset);
-				if (be16_to_cpu(vhdr->signature) == HFSPLUS_VOLHEAD_SIG) {
-					memcpy(vhdr, sbi->s_vhdr, sizeof(*vhdr));
-					mark_buffer_dirty(bh);
-					brelse(bh);
-				} else
-					printk(KERN_WARNING "hfs: backup not found!\n");
-			}
-		}
+		memcpy(sbi->s_backup_vhdr, sbi->s_vhdr, sizeof(*sbi->s_vhdr));
+		write_backup = 1;
 	}
+
+	error = hfsplus_submit_bio(sb->s_bdev,
+				   sbi->part_start + HFSPLUS_VOLHEAD_SECTOR,
+				   sbi->s_vhdr, WRITE_SYNC);
+	if (!write_backup)
+		goto out;
+
+	error2 = hfsplus_submit_bio(sb->s_bdev,
+				  sbi->part_start + sbi->sect_count - 2,
+				  sbi->s_backup_vhdr, WRITE_SYNC);
+	if (!error)
+		error2 = error;
+out:
 	mutex_unlock(&sbi->alloc_mutex);
 	mutex_unlock(&sbi->vh_mutex);
-	return 0;
+	return error;
 }
 
 static void hfsplus_write_super(struct super_block *sb)
@@ -229,7 +224,8 @@ static void hfsplus_put_super(struct sup
 	hfs_btree_close(sbi->ext_tree);
 	iput(sbi->alloc_file);
 	iput(sbi->hidden_dir);
-	brelse(sbi->s_vhbh);
+	kfree(sbi->s_vhdr);
+	kfree(sbi->s_backup_vhdr);
 	unload_nls(sbi->nls);
 	kfree(sb->s_fs_info);
 	sb->s_fs_info = NULL;
Index: linux-2.6/fs/hfsplus/hfsplus_fs.h
===================================================================
--- linux-2.6.orig/fs/hfsplus/hfsplus_fs.h	2010-11-17 22:45:16.992254297 +0100
+++ linux-2.6/fs/hfsplus/hfsplus_fs.h	2010-11-17 22:45:21.059004123 +0100
@@ -107,8 +107,8 @@ struct hfsplus_vh;
 struct hfs_btree;
 
 struct hfsplus_sb_info {
-	struct buffer_head *s_vhbh;
 	struct hfsplus_vh *s_vhdr;
+	struct hfsplus_vh *s_backup_vhdr;
 	struct hfs_btree *ext_tree;
 	struct hfs_btree *cat_tree;
 	struct hfs_btree *attr_tree;
@@ -118,7 +118,8 @@ struct hfsplus_sb_info {
 
 	/* Runtime variables */
 	u32 blockoffset;
-	u32 sect_count;
+	sector_t part_start;
+	sector_t sect_count;
 	int fs_shift;
 
 	/* immutable data from the volume header */
@@ -385,8 +386,9 @@ int hfsplus_compare_dentry(struct dentry
 
 /* wrapper.c */
 int hfsplus_read_wrapper(struct super_block *);
-
 int hfs_part_find(struct super_block *, sector_t *, sector_t *);
+int hfsplus_submit_bio(struct block_device *bdev, sector_t sector,
+		void *data, int rw);
 
 /* access macros */
 static inline struct hfsplus_sb_info *HFSPLUS_SB(struct super_block *sb)
Index: linux-2.6/fs/hfsplus/wrapper.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/wrapper.c	2010-11-17 22:45:17.001253738 +0100
+++ linux-2.6/fs/hfsplus/wrapper.c	2010-11-17 22:45:21.065004263 +0100
@@ -24,6 +24,40 @@ struct hfsplus_wd {
 	u16 embed_count;
 };
 
+static void hfsplus_end_io_sync(struct bio *bio, int err)
+{
+	if (err)
+		clear_bit(BIO_UPTODATE, &bio->bi_flags);
+	complete(bio->bi_private);
+}
+
+int hfsplus_submit_bio(struct block_device *bdev, sector_t sector,
+		void *data, int rw)
+{
+	DECLARE_COMPLETION_ONSTACK(wait);
+	struct bio *bio;
+
+	bio = bio_alloc(GFP_NOIO, 1);
+	bio->bi_sector = sector;
+	bio->bi_bdev = bdev;
+	bio->bi_end_io = hfsplus_end_io_sync;
+	bio->bi_private = &wait;
+
+	/*
+	 * We always submit one sector at a time, so bio_add_page must not fail.
+	 */
+	if (bio_add_page(bio, virt_to_page(data), HFSPLUS_SECTOR_SIZE,
+			 offset_in_page(data)) != HFSPLUS_SECTOR_SIZE)
+		BUG();
+
+	submit_bio(rw, bio);
+	wait_for_completion(&wait);
+
+	if (!bio_flagged(bio, BIO_UPTODATE))
+		return -EIO;
+	return 0;
+}
+
 static int hfsplus_read_mdb(void *bufptr, struct hfsplus_wd *wd)
 {
 	u32 extent;
@@ -88,100 +122,111 @@ static int hfsplus_get_last_session(stru
 int hfsplus_read_wrapper(struct super_block *sb)
 {
 	struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
-	struct buffer_head *bh;
-	struct hfsplus_vh *vhdr;
 	struct hfsplus_wd wd;
 	sector_t part_start, part_size;
 	u32 blocksize;
+	int error = 0;
 
+	error = -EINVAL;
 	blocksize = sb_min_blocksize(sb, HFSPLUS_SECTOR_SIZE);
 	if (!blocksize)
-		return -EINVAL;
+		goto out;
 
 	if (hfsplus_get_last_session(sb, &part_start, &part_size))
-		return -EINVAL;
+		goto out;
 	if ((u64)part_start + part_size > 0x100000000ULL) {
 		pr_err("hfs: volumes larger than 2TB are not supported yet\n");
-		return -EINVAL;
+		goto out;
 	}
-	while (1) {
-		bh = sb_bread512(sb, part_start + HFSPLUS_VOLHEAD_SECTOR, vhdr);
-		if (!bh)
-			return -EIO;
-
-		if (vhdr->signature == cpu_to_be16(HFSP_WRAP_MAGIC)) {
-			if (!hfsplus_read_mdb(vhdr, &wd))
-				goto error;
-			wd.ablk_size >>= HFSPLUS_SECTOR_SHIFT;
-			part_start += wd.ablk_start + wd.embed_start * wd.ablk_size;
-			part_size = wd.embed_count * wd.ablk_size;
-			brelse(bh);
-			bh = sb_bread512(sb, part_start + HFSPLUS_VOLHEAD_SECTOR, vhdr);
-			if (!bh)
-				return -EIO;
-		}
-		if (vhdr->signature == cpu_to_be16(HFSPLUS_VOLHEAD_SIG))
-			break;
-		if (vhdr->signature == cpu_to_be16(HFSPLUS_VOLHEAD_SIGX)) {
-			set_bit(HFSPLUS_SB_HFSX, &sbi->flags);
-			break;
-		}
-		brelse(bh);
 
-		/* check for a partition block
+	error = -ENOMEM;
+	sbi->s_vhdr = kmalloc(HFSPLUS_SECTOR_SIZE, GFP_KERNEL);
+	if (!sbi->s_vhdr)
+		goto out;
+	sbi->s_backup_vhdr = kmalloc(HFSPLUS_SECTOR_SIZE, GFP_KERNEL);
+	if (!sbi->s_backup_vhdr)
+		goto out_free_vhdr;
+
+reread:
+	error = hfsplus_submit_bio(sb->s_bdev,
+				   part_start + HFSPLUS_VOLHEAD_SECTOR,
+				   sbi->s_vhdr, READ);
+	if (error)
+		goto out_free_backup_vhdr;
+
+	error = -EINVAL;
+	switch (sbi->s_vhdr->signature) {
+	case cpu_to_be16(HFSPLUS_VOLHEAD_SIGX):
+		set_bit(HFSPLUS_SB_HFSX, &sbi->flags);
+		/*FALLTHRU*/
+	case cpu_to_be16(HFSPLUS_VOLHEAD_SIG):
+		break;
+	case cpu_to_be16(HFSP_WRAP_MAGIC):
+		if (!hfsplus_read_mdb(sbi->s_vhdr, &wd))
+			goto out;
+		wd.ablk_size >>= HFSPLUS_SECTOR_SHIFT;
+		part_start += wd.ablk_start + wd.embed_start * wd.ablk_size;
+		part_size = wd.embed_count * wd.ablk_size;
+		goto reread;
+	default:
+		/*
+		 * Check for a partition block.
+		 *
 		 * (should do this only for cdrom/loop though)
 		 */
 		if (hfs_part_find(sb, &part_start, &part_size))
-			return -EINVAL;
+			goto out;
+		goto reread;
+	}
+
+	error = hfsplus_submit_bio(sb->s_bdev,
+				   part_start + part_size - 2,
+				   sbi->s_backup_vhdr, READ);
+	if (error)
+		goto out_free_backup_vhdr;
+
+	error = -EINVAL;
+	if (sbi->s_backup_vhdr->signature != sbi->s_vhdr->signature) {
+		printk(KERN_WARNING
+			"hfs: invalid secondary volume header\n");
+		goto out_free_backup_vhdr;
 	}
 
-	blocksize = be32_to_cpu(vhdr->blocksize);
-	brelse(bh);
+	blocksize = be32_to_cpu(sbi->s_vhdr->blocksize);
 
-	/* block size must be at least as large as a sector
-	 * and a multiple of 2
+	/*
+	 * Block size must be at least as large as a sector and a multiple of 2.
 	 */
-	if (blocksize < HFSPLUS_SECTOR_SIZE ||
-	    ((blocksize - 1) & blocksize))
-		return -EINVAL;
+	if (blocksize < HFSPLUS_SECTOR_SIZE || ((blocksize - 1) & blocksize))
+		goto out_free_backup_vhdr;
 	sbi->alloc_blksz = blocksize;
 	sbi->alloc_blksz_shift = 0;
 	while ((blocksize >>= 1) != 0)
 		sbi->alloc_blksz_shift++;
 	blocksize = min(sbi->alloc_blksz, (u32)PAGE_SIZE);
 
-	/* align block size to block offset */
+	/*
+	 * Align block size to block offset.
+	 */
 	while (part_start & ((blocksize >> HFSPLUS_SECTOR_SHIFT) - 1))
 		blocksize >>= 1;
 
 	if (sb_set_blocksize(sb, blocksize) != blocksize) {
 		printk(KERN_ERR "hfs: unable to set blocksize to %u!\n", blocksize);
-		return -EINVAL;
+		goto out_free_backup_vhdr;
 	}
 
 	sbi->blockoffset =
 		part_start >> (sb->s_blocksize_bits - HFSPLUS_SECTOR_SHIFT);
+	sbi->part_start = part_start;
 	sbi->sect_count = part_size;
 	sbi->fs_shift = sbi->alloc_blksz_shift - sb->s_blocksize_bits;
-
-	bh = sb_bread512(sb, part_start + HFSPLUS_VOLHEAD_SECTOR, vhdr);
-	if (!bh)
-		return -EIO;
-
-	/* should still be the same... */
-	if (test_bit(HFSPLUS_SB_HFSX, &sbi->flags)) {
-		if (vhdr->signature != cpu_to_be16(HFSPLUS_VOLHEAD_SIGX))
-			goto error;
-	} else {
-		if (vhdr->signature != cpu_to_be16(HFSPLUS_VOLHEAD_SIG))
-			goto error;
-	}
-
-	sbi->s_vhbh = bh;
-	sbi->s_vhdr = vhdr;
-
 	return 0;
- error:
-	brelse(bh);
-	return -EINVAL;
+
+out_free_backup_vhdr:
+	kfree(sbi->s_backup_vhdr);
+out_free_vhdr:
+	kfree(sbi->s_vhdr);
+out:
+	return error;
 }

[PATCH 4/11] hfsplus: use raw bio access for partition tables

[Christoph Hellwig]

Switch the hfsplus partition table reding for cdroms to use our bio
helpers.  Again we don't rely on any caching in the buffer_heads, and
this gets rid of the last buffer_head use in hfsplus.

Signed-off-by: Christoph Hellwig <hch@tuxera.com>

Index: linux-2.6/fs/hfsplus/part_tbl.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/part_tbl.c	2010-11-17 22:43:32.005254018 +0100
+++ linux-2.6/fs/hfsplus/part_tbl.c	2010-11-17 22:44:20.827005379 +0100
@@ -13,6 +13,7 @@
  *
  */
 
+#include <linux/slab.h>
 #include "hfsplus_fs.h"
 
 /* offsets to various blocks */
@@ -65,70 +66,87 @@ struct old_pmap {
 	}	pdEntry[42];
 } __packed;
 
+static int hfs_parse_old_pmap(struct super_block *sb, struct old_pmap *pm,
+		sector_t *part_start, sector_t *part_size)
+{
+	struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
+	int i;
+
+	for (i = 0; i < 42; i++) {
+		struct old_pmap_entry *p = &pm->pdEntry[i];
+
+		if (p->pdStart && p->pdSize &&
+		    p->pdFSID == cpu_to_be32(0x54465331)/*"TFS1"*/ &&
+		    (sbi->part < 0 || sbi->part == i)) {
+			*part_start += be32_to_cpu(p->pdStart);
+			*part_size = be32_to_cpu(p->pdSize);
+			return 0;
+		}
+	}
+
+	return -ENOENT;
+}
+
+static int hfs_parse_new_pmap(struct super_block *sb, struct new_pmap *pm,
+		sector_t *part_start, sector_t *part_size)
+{
+	struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
+	int size = be32_to_cpu(pm->pmMapBlkCnt);
+	int res;
+	int i = 0;
+
+	do {
+		if (!memcmp(pm->pmPartType,"Apple_HFS", 9) &&
+		    (sbi->part < 0 || sbi->part == i)) {
+			*part_start += be32_to_cpu(pm->pmPyPartStart);
+			*part_size = be32_to_cpu(pm->pmPartBlkCnt);
+			return 0;
+		}
+
+		if (++i >= size)
+			return -ENOENT;
+
+		res = hfsplus_submit_bio(sb->s_bdev,
+					 *part_start + HFS_PMAP_BLK + i,
+					 pm, READ);
+		if (res)
+			return res;
+	} while (pm->pmSig == cpu_to_be16(HFS_NEW_PMAP_MAGIC));
+
+	return -ENOENT;
+}
+
 /*
- * hfs_part_find()
- *
- * Parse the partition map looking for the
- * start and length of the 'part'th HFS partition.
+ * Parse the partition map looking for the start and length of a
+ * HFS/HFS+ partition.
  */
 int hfs_part_find(struct super_block *sb,
-		  sector_t *part_start, sector_t *part_size)
+		sector_t *part_start, sector_t *part_size)
 {
-	struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
-	struct buffer_head *bh;
-	__be16 *data;
-	int i, size, res;
-
-	res = -ENOENT;
-	bh = sb_bread512(sb, *part_start + HFS_PMAP_BLK, data);
-	if (!bh)
-		return -EIO;
+	void *data;
+	int res;
+
+	data = kmalloc(HFSPLUS_SECTOR_SIZE, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	res = hfsplus_submit_bio(sb->s_bdev, *part_start + HFS_PMAP_BLK,
+				 data, READ);
+	if (res)
+		return res;
 
-	switch (be16_to_cpu(*data)) {
+	switch (be16_to_cpu(*((__be16 *)data))) {
 	case HFS_OLD_PMAP_MAGIC:
-	  {
-		struct old_pmap *pm;
-		struct old_pmap_entry *p;
-
-		pm = (struct old_pmap *)bh->b_data;
-		p = pm->pdEntry;
-		size = 42;
-		for (i = 0; i < size; p++, i++) {
-			if (p->pdStart && p->pdSize &&
-			    p->pdFSID == cpu_to_be32(0x54465331)/*"TFS1"*/ &&
-			    (sbi->part < 0 || sbi->part == i)) {
-				*part_start += be32_to_cpu(p->pdStart);
-				*part_size = be32_to_cpu(p->pdSize);
-				res = 0;
-			}
-		}
+		res = hfs_parse_old_pmap(sb, data, part_start, part_size);
 		break;
-	  }
 	case HFS_NEW_PMAP_MAGIC:
-	  {
-		struct new_pmap *pm;
-
-		pm = (struct new_pmap *)bh->b_data;
-		size = be32_to_cpu(pm->pmMapBlkCnt);
-		for (i = 0; i < size;) {
-			if (!memcmp(pm->pmPartType,"Apple_HFS", 9) &&
-			    (sbi->part < 0 || sbi->part == i)) {
-				*part_start += be32_to_cpu(pm->pmPyPartStart);
-				*part_size = be32_to_cpu(pm->pmPartBlkCnt);
-				res = 0;
-				break;
-			}
-			brelse(bh);
-			bh = sb_bread512(sb, *part_start + HFS_PMAP_BLK + ++i, pm);
-			if (!bh)
-				return -EIO;
-			if (pm->pmSig != cpu_to_be16(HFS_NEW_PMAP_MAGIC))
-				break;
-		}
+		res = hfs_parse_new_pmap(sb, data, part_start, part_size);
+		break;
+	default:
+		res = -ENOENT;
 		break;
-	  }
 	}
-	brelse(bh);
 
+	kfree(data);
 	return res;
 }
Index: linux-2.6/fs/hfsplus/hfsplus_fs.h
===================================================================
--- linux-2.6.orig/fs/hfsplus/hfsplus_fs.h	2010-11-17 22:43:32.011254158 +0100
+++ linux-2.6/fs/hfsplus/hfsplus_fs.h	2010-11-17 22:44:20.828003913 +0100
@@ -401,23 +401,6 @@ static inline struct hfsplus_inode_info
 	return list_entry(inode, struct hfsplus_inode_info, vfs_inode);
 }
 
-#define sb_bread512(sb, sec, data) ({			\
-	struct buffer_head *__bh;			\
-	sector_t __block;				\
-	loff_t __start;					\
-	int __offset;					\
-							\
-	__start = (loff_t)(sec) << HFSPLUS_SECTOR_SHIFT;\
-	__block = __start >> (sb)->s_blocksize_bits;	\
-	__offset = __start & ((sb)->s_blocksize - 1);	\
-	__bh = sb_bread((sb), __block);			\
-	if (likely(__bh != NULL))			\
-		data = (void *)(__bh->b_data + __offset);\
-	else						\
-		data = NULL;				\
-	__bh;						\
-})
-
 /* time macros */
 #define __hfsp_mt2ut(t)		(be32_to_cpu(t) - 2082844800U)
 #define __hfsp_ut2mt(t)		(cpu_to_be32(t + 2082844800U))

[PATCH 5/11] hfsplus: make sure sync writes out all metadata

[Christoph Hellwig]

hfsplus stores all metadata except for the volume headers in special
inodes.  While these are marked hashed and periodically written out
by the flusher threads, we can't rely on that for sync.  For the case
of a data integrity sync the VM has life-lock avoidance code that
avoids writing inodes again that are redirtied during the sync,
which is something that can happen easily for hfsplus.  So make sure
we explicitly write out the metadata inodes at the beginning of
hfsplus_sync_fs.

Signed-off-by: Christoph Hellwig <hch@tuxera.com>

Index: linux-2.6/fs/hfsplus/super.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/super.c	2010-11-17 22:46:55.000000000 +0100
+++ linux-2.6/fs/hfsplus/super.c	2010-11-17 22:47:57.762004821 +0100
@@ -164,6 +164,22 @@ int hfsplus_sync_fs(struct super_block *
 
 	sb->s_dirt = 0;
 
+	/*
+	 * Explicitly write out the special metadata inodes.
+	 *
+	 * While these special inodes are marked as hashed and written
+	 * out peridocically by the flusher threads we redirty them
+	 * during writeout of normal inodes, and thus the life lock
+	 * prevents us from getting the latest state to disk.
+	 */
+	error = filemap_write_and_wait(sbi->cat_tree->inode->i_mapping);
+	error2 = filemap_write_and_wait(sbi->ext_tree->inode->i_mapping);
+	if (!error)
+		error = error2;
+	error2 = filemap_write_and_wait(sbi->alloc_file->i_mapping);
+	if (!error)
+		error = error2;
+
 	mutex_lock(&sbi->vh_mutex);
 	mutex_lock(&sbi->alloc_mutex);
 	vhdr->free_blocks = cpu_to_be32(sbi->free_blocks);
@@ -176,9 +192,11 @@ int hfsplus_sync_fs(struct super_block *
 		write_backup = 1;
 	}
 
-	error = hfsplus_submit_bio(sb->s_bdev,
+	error2 = hfsplus_submit_bio(sb->s_bdev,
 				   sbi->part_start + HFSPLUS_VOLHEAD_SECTOR,
 				   sbi->s_vhdr, WRITE_SYNC);
+	if (!error)
+		error = error2;
 	if (!write_backup)
 		goto out;
 

[PATCH 6/11] hfsplus: avoid useless work in hfsplus_sync_fs

[Christoph Hellwig]

There is no reason to write out the metadata inodes or volume headers
during a non-blocking sync, as we are almost guaranteed to dirty them
again during the inode writeouts.

Signed-off-by: Christoph Hellwig <hch@tuxera.com>

Index: linux-2.6/fs/hfsplus/super.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/super.c	2010-11-17 22:47:57.762004821 +0100
+++ linux-2.6/fs/hfsplus/super.c	2010-11-17 22:50:17.024003983 +0100
@@ -160,6 +160,9 @@ int hfsplus_sync_fs(struct super_block *
 	int write_backup = 0;
 	int error, error2;
 
+	if (!wait)
+		return 0;
+
 	dprint(DBG_SUPER, "hfsplus_write_super\n");
 
 	sb->s_dirt = 0;

[PATCH 7/11] hfsplus: simplify fsync

[Christoph Hellwig]

Remove lots of code we don't need from fsync, we just need to call
->write_inode on the inode if it's dirty, for which sync_inode_metadata
is a lot more efficient than write_inode_now, and we need to write
out the various metadata inodes, which we now do explicitly instead
of by calling ->sync_fs.

Signed-off-by: Christoph Hellwig <hch@tuxera.com>

Index: linux-2.6/fs/hfsplus/inode.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/inode.c	2010-11-17 22:43:05.733007195 +0100
+++ linux-2.6/fs/hfsplus/inode.c	2010-11-17 22:51:49.999003913 +0100
@@ -302,29 +302,28 @@ static int hfsplus_setattr(struct dentry
 	return 0;
 }
 
-static int hfsplus_file_fsync(struct file *filp, int datasync)
+static int hfsplus_file_fsync(struct file *file, int datasync)
 {
-	struct inode *inode = filp->f_mapping->host;
-	struct super_block * sb;
-	int ret, err;
+	struct inode *inode = file->f_mapping->host;
+	struct hfsplus_sb_info *sbi = HFSPLUS_SB(inode->i_sb);
+	int error, error2;
 
-	/* sync the inode to buffers */
-	ret = write_inode_now(inode, 0);
+	/*
+	 * Sync inode metadata into the catalog and extent trees.
+	 */
+	sync_inode_metadata(inode, 1);
 
-	/* sync the superblock to buffers */
-	sb = inode->i_sb;
-	if (sb->s_dirt) {
-		if (!(sb->s_flags & MS_RDONLY))
-			hfsplus_sync_fs(sb, 1);
-		else
-			sb->s_dirt = 0;
-	}
-
-	/* .. finally sync the buffers to disk */
-	err = sync_blockdev(sb->s_bdev);
-	if (!ret)
-		ret = err;
-	return ret;
+	/*
+	 * And explicitly write out the btrees.
+	 */
+	error = filemap_write_and_wait(sbi->cat_tree->inode->i_mapping);
+	error2 = filemap_write_and_wait(sbi->ext_tree->inode->i_mapping);
+	if (!error)
+		error = error2;
+	error2 = filemap_write_and_wait(sbi->alloc_file->i_mapping);
+	if (!error)
+		error = error2;
+	return error;
 }
 
 static const struct inode_operations hfsplus_file_inode_operations = {

[PATCH 8/11] hfsplus: write up fsync for directories

[Christoph Hellwig]

fsync is supposed to not just work on regular files, but also on
directories.  Fortunately enough hfsplus_file_fsync works just fine
for directories, so we can just wire it up.

Signed-off-by: Christoph Hellwig <hch@tuxera.com>

Index: linux-2.6/fs/hfsplus/dir.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/dir.c	2010-11-17 17:56:34.456022981 +0100
+++ linux-2.6/fs/hfsplus/dir.c	2010-11-17 17:57:05.398003633 +0100
@@ -485,6 +485,7 @@ const struct inode_operations hfsplus_di
 };
 
 const struct file_operations hfsplus_dir_operations = {
+	.fsync		= hfsplus_file_fsync,
 	.read		= generic_read_dir,
 	.readdir	= hfsplus_readdir,
 	.unlocked_ioctl = hfsplus_ioctl,
Index: linux-2.6/fs/hfsplus/hfsplus_fs.h
===================================================================
--- linux-2.6.orig/fs/hfsplus/hfsplus_fs.h	2010-11-17 17:56:34.443004473 +0100
+++ linux-2.6/fs/hfsplus/hfsplus_fs.h	2010-11-17 17:56:50.548003984 +0100
@@ -352,6 +352,7 @@ int hfsplus_cat_read_inode(struct inode
 int hfsplus_cat_write_inode(struct inode *);
 struct inode *hfsplus_new_inode(struct super_block *, int);
 void hfsplus_delete_inode(struct inode *);
+int hfsplus_file_fsync(struct file *file, int datasync);
 
 /* ioctl.c */
 long hfsplus_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
Index: linux-2.6/fs/hfsplus/inode.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/inode.c	2010-11-17 17:56:34.429008943 +0100
+++ linux-2.6/fs/hfsplus/inode.c	2010-11-17 17:56:39.059006079 +0100
@@ -302,7 +302,7 @@ static int hfsplus_setattr(struct dentry
 	return 0;
 }
 
-static int hfsplus_file_fsync(struct file *file, int datasync)
+int hfsplus_file_fsync(struct file *file, int datasync)
 {
 	struct inode *inode = file->f_mapping->host;
 	struct hfsplus_sb_info *sbi = HFSPLUS_SB(inode->i_sb);

[PATCH 9/11] hfsplus: split up inode flags

[Christoph Hellwig]

Split the flags field in the hfsplus inode into an extent_state
flag that is locked by the extent_lock, and a new flags field
that uses atomic bitops.  The second will grow more flags in the
next patch.

Signed-off-by: Christoph Hellwig <hch@tuxera.com>

Index: linux-2.6/fs/hfsplus/inode.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/inode.c	2010-11-17 17:58:32.545004262 +0100
+++ linux-2.6/fs/hfsplus/inode.c	2010-11-17 17:58:46.460003005 +0100
@@ -190,7 +190,9 @@ static struct dentry *hfsplus_file_looku
 	inode->i_ino = dir->i_ino;
 	INIT_LIST_HEAD(&hip->open_dir_list);
 	mutex_init(&hip->extents_lock);
-	hip->flags = HFSPLUS_FLG_RSRC;
+	hip->extent_state = 0;
+	hip->flags = 0;
+	set_bit(HFSPLUS_I_RSRC, &hip->flags);
 
 	hfs_find_init(HFSPLUS_SB(sb)->cat_tree, &fd);
 	err = hfsplus_find_cat(sb, dir->i_ino, &fd);
@@ -369,6 +371,7 @@ struct inode *hfsplus_new_inode(struct s
 	INIT_LIST_HEAD(&hip->open_dir_list);
 	mutex_init(&hip->extents_lock);
 	atomic_set(&hip->opencnt, 0);
+	hip->extent_state = 0;
 	hip->flags = 0;
 	memset(hip->first_extents, 0, sizeof(hfsplus_extent_rec));
 	memset(hip->cached_extents, 0, sizeof(hfsplus_extent_rec));
@@ -498,8 +501,8 @@ int hfsplus_cat_read_inode(struct inode
 		hfs_bnode_read(fd->bnode, &entry, fd->entryoffset,
 					sizeof(struct hfsplus_cat_file));
 
-		hfsplus_inode_read_fork(inode, HFSPLUS_IS_DATA(inode) ?
-					&file->data_fork : &file->rsrc_fork);
+		hfsplus_inode_read_fork(inode, HFSPLUS_IS_RSRC(inode) ?
+					&file->rsrc_fork : &file->data_fork);
 		hfsplus_get_perms(inode, &file->permissions, 0);
 		inode->i_nlink = 1;
 		if (S_ISREG(inode->i_mode)) {
Index: linux-2.6/fs/hfsplus/hfsplus_fs.h
===================================================================
--- linux-2.6.orig/fs/hfsplus/hfsplus_fs.h	2010-11-17 17:58:32.540003494 +0100
+++ linux-2.6/fs/hfsplus/hfsplus_fs.h	2010-11-17 17:58:46.461005519 +0100
@@ -171,7 +171,7 @@ struct hfsplus_inode_info {
 	u32 cached_blocks;
 	hfsplus_extent_rec first_extents;
 	hfsplus_extent_rec cached_extents;
-	unsigned long flags;
+	unsigned int extent_state;
 	struct mutex extents_lock;
 
 	/*
@@ -186,6 +186,11 @@ struct hfsplus_inode_info {
 	u32 linkid;
 
 	/*
+	 * Accessed using atomic bitops.
+	 */
+	unsigned long flags;
+
+	/*
 	 * Protected by i_mutex.
 	 */
 	sector_t fs_blocks;
@@ -196,12 +201,13 @@ struct hfsplus_inode_info {
 	struct inode vfs_inode;
 };
 
-#define HFSPLUS_FLG_RSRC	0x0001
-#define HFSPLUS_FLG_EXT_DIRTY	0x0002
-#define HFSPLUS_FLG_EXT_NEW	0x0004
+#define HFSPLUS_EXT_DIRTY	0x0001
+#define HFSPLUS_EXT_NEW		0x0002
+
+#define HFSPLUS_I_RSRC		0	/* represents a resource fork */
 
-#define HFSPLUS_IS_DATA(inode)   (!(HFSPLUS_I(inode)->flags & HFSPLUS_FLG_RSRC))
-#define HFSPLUS_IS_RSRC(inode)   (HFSPLUS_I(inode)->flags & HFSPLUS_FLG_RSRC)
+#define HFSPLUS_IS_RSRC(inode) \
+	test_bit(HFSPLUS_I_RSRC, &HFSPLUS_I(inode)->flags)
 
 struct hfs_find_data {
 	/* filled by caller */
Index: linux-2.6/fs/hfsplus/super.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/super.c	2010-11-17 17:58:30.862004052 +0100
+++ linux-2.6/fs/hfsplus/super.c	2010-11-17 17:58:47.392004821 +0100
@@ -66,6 +66,7 @@ struct inode *hfsplus_iget(struct super_
 	INIT_LIST_HEAD(&HFSPLUS_I(inode)->open_dir_list);
 	mutex_init(&HFSPLUS_I(inode)->extents_lock);
 	HFSPLUS_I(inode)->flags = 0;
+	HFSPLUS_I(inode)->extent_state = 0;
 	HFSPLUS_I(inode)->rsrc_inode = NULL;
 	atomic_set(&HFSPLUS_I(inode)->opencnt, 0);
 
Index: linux-2.6/fs/hfsplus/extents.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/extents.c	2010-11-17 17:58:24.552254856 +0100
+++ linux-2.6/fs/hfsplus/extents.c	2010-11-17 17:58:58.357254087 +0100
@@ -95,24 +95,24 @@ static void __hfsplus_ext_write_extent(s
 				HFSPLUS_TYPE_RSRC : HFSPLUS_TYPE_DATA);
 
 	res = hfs_brec_find(fd);
-	if (hip->flags & HFSPLUS_FLG_EXT_NEW) {
+	if (hip->extent_state & HFSPLUS_EXT_NEW) {
 		if (res != -ENOENT)
 			return;
 		hfs_brec_insert(fd, hip->cached_extents,
 				sizeof(hfsplus_extent_rec));
-		hip->flags &= ~(HFSPLUS_FLG_EXT_DIRTY | HFSPLUS_FLG_EXT_NEW);
+		hip->extent_state &= ~(HFSPLUS_EXT_DIRTY | HFSPLUS_EXT_NEW);
 	} else {
 		if (res)
 			return;
 		hfs_bnode_write(fd->bnode, hip->cached_extents,
 				fd->entryoffset, fd->entrylength);
-		hip->flags &= ~HFSPLUS_FLG_EXT_DIRTY;
+		hip->extent_state &= ~HFSPLUS_EXT_DIRTY;
 	}
 }
 
 static void hfsplus_ext_write_extent_locked(struct inode *inode)
 {
-	if (HFSPLUS_I(inode)->flags & HFSPLUS_FLG_EXT_DIRTY) {
+	if (HFSPLUS_I(inode)->extent_state & HFSPLUS_EXT_DIRTY) {
 		struct hfs_find_data fd;
 
 		hfs_find_init(HFSPLUS_SB(inode->i_sb)->ext_tree, &fd);
@@ -155,7 +155,7 @@ static inline int __hfsplus_ext_cache_ex
 
 	WARN_ON(!mutex_is_locked(&hip->extents_lock));
 
-	if (hip->flags & HFSPLUS_FLG_EXT_DIRTY)
+	if (hip->extent_state & HFSPLUS_EXT_DIRTY)
 		__hfsplus_ext_write_extent(inode, fd);
 
 	res = __hfsplus_ext_read_extent(fd, hip->cached_extents, inode->i_ino,
@@ -167,7 +167,7 @@ static inline int __hfsplus_ext_cache_ex
 		hip->cached_blocks = hfsplus_ext_block_count(hip->cached_extents);
 	} else {
 		hip->cached_start = hip->cached_blocks = 0;
-		hip->flags &= ~(HFSPLUS_FLG_EXT_DIRTY | HFSPLUS_FLG_EXT_NEW);
+		hip->extent_state &= ~(HFSPLUS_EXT_DIRTY | HFSPLUS_EXT_NEW);
 	}
 	return res;
 }
@@ -429,7 +429,7 @@ int hfsplus_file_extend(struct inode *in
 					 start, len);
 		if (!res) {
 			hfsplus_dump_extent(hip->cached_extents);
-			hip->flags |= HFSPLUS_FLG_EXT_DIRTY;
+			hip->extent_state |= HFSPLUS_EXT_DIRTY;
 			hip->cached_blocks += len;
 		} else if (res == -ENOSPC)
 			goto insert_extent;
@@ -450,7 +450,7 @@ insert_extent:
 	hip->cached_extents[0].start_block = cpu_to_be32(start);
 	hip->cached_extents[0].block_count = cpu_to_be32(len);
 	hfsplus_dump_extent(hip->cached_extents);
-	hip->flags |= HFSPLUS_FLG_EXT_DIRTY | HFSPLUS_FLG_EXT_NEW;
+	hip->extent_state |= HFSPLUS_EXT_DIRTY | HFSPLUS_EXT_NEW;
 	hip->cached_start = hip->alloc_blocks;
 	hip->cached_blocks = len;
 
@@ -513,12 +513,12 @@ void hfsplus_file_truncate(struct inode
 				     alloc_cnt - start, alloc_cnt - blk_cnt);
 		hfsplus_dump_extent(hip->cached_extents);
 		if (blk_cnt > start) {
-			hip->flags |= HFSPLUS_FLG_EXT_DIRTY;
+			hip->extent_state |= HFSPLUS_EXT_DIRTY;
 			break;
 		}
 		alloc_cnt = start;
 		hip->cached_start = hip->cached_blocks = 0;
-		hip->flags &= ~(HFSPLUS_FLG_EXT_DIRTY | HFSPLUS_FLG_EXT_NEW);
+		hip->extent_state &= ~(HFSPLUS_EXT_DIRTY | HFSPLUS_EXT_NEW);
 		hfs_brec_remove(&fd);
 	}
 	hfs_find_exit(&fd);

[PATCH 10/11] hfsplus: optimize fsync

[Christoph Hellwig]

Avoid doing unessecary work in fsync.  Do nothing unless the inode
was marked dirty, and only write the various metadata inodes out if
they contain any dirty state from this inode.  This is archived by
adding three new dirty bits to the hfsplus-specific inode which are
set in the correct places.

Signed-off-by: Christoph Hellwig <hch@tuxera.com>

Index: linux-2.6/fs/hfsplus/inode.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/inode.c	2010-11-17 22:54:53.842254646 +0100
+++ linux-2.6/fs/hfsplus/inode.c	2010-11-17 22:56:01.488273225 +0100
@@ -307,8 +307,12 @@ static int hfsplus_setattr(struct dentry
 int hfsplus_file_fsync(struct file *file, int datasync)
 {
 	struct inode *inode = file->f_mapping->host;
+	struct hfsplus_inode_info *hip = HFSPLUS_I(inode);
 	struct hfsplus_sb_info *sbi = HFSPLUS_SB(inode->i_sb);
-	int error, error2;
+	int error = 0, error2;
+
+	if (!(inode->i_state & I_DIRTY))
+		return 0;
 
 	/*
 	 * Sync inode metadata into the catalog and extent trees.
@@ -318,13 +322,21 @@ int hfsplus_file_fsync(struct file *file
 	/*
 	 * And explicitly write out the btrees.
 	 */
-	error = filemap_write_and_wait(sbi->cat_tree->inode->i_mapping);
-	error2 = filemap_write_and_wait(sbi->ext_tree->inode->i_mapping);
-	if (!error)
-		error = error2;
-	error2 = filemap_write_and_wait(sbi->alloc_file->i_mapping);
-	if (!error)
-		error = error2;
+	if (test_and_clear_bit(HFSPLUS_I_CAT_DIRTY, &hip->flags))
+		error = filemap_write_and_wait(sbi->cat_tree->inode->i_mapping);
+
+	if (test_and_clear_bit(HFSPLUS_I_EXT_DIRTY, &hip->flags)) {
+		error2 = filemap_write_and_wait(sbi->ext_tree->inode->i_mapping);
+		if (!error)
+			error = error2;
+	}
+
+	if (test_and_clear_bit(HFSPLUS_I_ALLOC_DIRTY, &hip->flags)) {
+		error2 = filemap_write_and_wait(sbi->alloc_file->i_mapping);
+		if (!error)
+			error = error2;
+	}
+
 	return error;
 }
 
@@ -590,6 +602,8 @@ int hfsplus_cat_write_inode(struct inode
 		hfs_bnode_write(fd.bnode, &entry, fd.entryoffset,
 					 sizeof(struct hfsplus_cat_file));
 	}
+
+	set_bit(HFSPLUS_I_CAT_DIRTY, &HFSPLUS_I(inode)->flags);
 out:
 	hfs_find_exit(&fd);
 	return 0;
Index: linux-2.6/fs/hfsplus/hfsplus_fs.h
===================================================================
--- linux-2.6.orig/fs/hfsplus/hfsplus_fs.h	2010-11-17 22:54:53.843254576 +0100
+++ linux-2.6/fs/hfsplus/hfsplus_fs.h	2010-11-17 22:56:01.488273225 +0100
@@ -157,6 +157,11 @@ struct hfsplus_sb_info {
 #define HFSPLUS_SB_HFSX		3
 #define HFSPLUS_SB_CASEFOLD	4
 
+static inline struct hfsplus_sb_info *HFSPLUS_SB(struct super_block *sb)
+{
+	return sb->s_fs_info;
+}
+
 
 struct hfsplus_inode_info {
 	atomic_t opencnt;
@@ -205,10 +210,24 @@ struct hfsplus_inode_info {
 #define HFSPLUS_EXT_NEW		0x0002
 
 #define HFSPLUS_I_RSRC		0	/* represents a resource fork */
+#define HFSPLUS_I_CAT_DIRTY	1	/* has changes in the catalog tree */
+#define HFSPLUS_I_EXT_DIRTY	2	/* has changes in the extent tree */
+#define HFSPLUS_I_ALLOC_DIRTY	3	/* has changes in the allocation file */
 
 #define HFSPLUS_IS_RSRC(inode) \
 	test_bit(HFSPLUS_I_RSRC, &HFSPLUS_I(inode)->flags)
 
+static inline struct hfsplus_inode_info *HFSPLUS_I(struct inode *inode)
+{
+	return list_entry(inode, struct hfsplus_inode_info, vfs_inode);
+}
+
+static inline void hfsplus_mark_catalog_dirty(struct inode *inode)
+{
+	set_bit(HFSPLUS_I_CAT_DIRTY, &HFSPLUS_I(inode)->flags);
+	mark_inode_dirty(inode);
+}
+
 struct hfs_find_data {
 	/* filled by caller */
 	hfsplus_btree_key *search_key;
@@ -397,17 +416,6 @@ int hfs_part_find(struct super_block *,
 int hfsplus_submit_bio(struct block_device *bdev, sector_t sector,
 		void *data, int rw);
 
-/* access macros */
-static inline struct hfsplus_sb_info *HFSPLUS_SB(struct super_block *sb)
-{
-	return sb->s_fs_info;
-}
-
-static inline struct hfsplus_inode_info *HFSPLUS_I(struct inode *inode)
-{
-	return list_entry(inode, struct hfsplus_inode_info, vfs_inode);
-}
-
 /* time macros */
 #define __hfsp_mt2ut(t)		(be32_to_cpu(t) - 2082844800U)
 #define __hfsp_ut2mt(t)		(cpu_to_be32(t + 2082844800U))
Index: linux-2.6/fs/hfsplus/catalog.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/catalog.c	2010-11-17 22:43:05.439004891 +0100
+++ linux-2.6/fs/hfsplus/catalog.c	2010-11-17 22:56:01.490026123 +0100
@@ -227,7 +227,8 @@ int hfsplus_create_cat(u32 cnid, struct
 
 	dir->i_size++;
 	dir->i_mtime = dir->i_ctime = CURRENT_TIME_SEC;
-	mark_inode_dirty(dir);
+	hfsplus_mark_catalog_dirty(dir);
+
 	hfs_find_exit(&fd);
 	return 0;
 
@@ -308,7 +309,7 @@ int hfsplus_delete_cat(u32 cnid, struct
 
 	dir->i_size--;
 	dir->i_mtime = dir->i_ctime = CURRENT_TIME_SEC;
-	mark_inode_dirty(dir);
+	hfsplus_mark_catalog_dirty(dir);
 out:
 	hfs_find_exit(&fd);
 
@@ -353,7 +354,6 @@ int hfsplus_rename_cat(u32 cnid,
 		goto out;
 	dst_dir->i_size++;
 	dst_dir->i_mtime = dst_dir->i_ctime = CURRENT_TIME_SEC;
-	mark_inode_dirty(dst_dir);
 
 	/* finally remove the old entry */
 	hfsplus_cat_build_key(sb, src_fd.search_key, src_dir->i_ino, src_name);
@@ -365,7 +365,6 @@ int hfsplus_rename_cat(u32 cnid,
 		goto out;
 	src_dir->i_size--;
 	src_dir->i_mtime = src_dir->i_ctime = CURRENT_TIME_SEC;
-	mark_inode_dirty(src_dir);
 
 	/* remove old thread entry */
 	hfsplus_cat_build_key(sb, src_fd.search_key, cnid, NULL);
@@ -387,6 +386,9 @@ int hfsplus_rename_cat(u32 cnid,
 		goto out;
 	}
 	err = hfs_brec_insert(&dst_fd, &entry, entry_size);
+
+	hfsplus_mark_catalog_dirty(dst_dir);
+	hfsplus_mark_catalog_dirty(src_dir);
 out:
 	hfs_bnode_put(dst_fd.bnode);
 	hfs_find_exit(&src_fd);
Index: linux-2.6/fs/hfsplus/extents.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/extents.c	2010-11-17 22:54:53.850253110 +0100
+++ linux-2.6/fs/hfsplus/extents.c	2010-11-17 22:56:01.494045889 +0100
@@ -108,6 +108,7 @@ static void __hfsplus_ext_write_extent(s
 				fd->entryoffset, fd->entrylength);
 		hip->extent_state &= ~HFSPLUS_EXT_DIRTY;
 	}
+	set_bit(HFSPLUS_I_EXT_DIRTY, &hip->flags);
 }
 
 static void hfsplus_ext_write_extent_locked(struct inode *inode)
@@ -403,6 +404,7 @@ int hfsplus_file_extend(struct inode *in
 	}
 
 	dprint(DBG_EXTENT, "extend %lu: %u,%u\n", inode->i_ino, start, len);
+	set_bit(HFSPLUS_I_ALLOC_DIRTY, &hip->flags);
 
 	if (hip->alloc_blocks <= hip->first_blocks) {
 		if (!hip->first_blocks) {
@@ -530,4 +532,5 @@ out:
 	hip->fs_blocks = (inode->i_size + sb->s_blocksize - 1) >> sb->s_blocksize_bits;
 	inode_set_bytes(inode, hip->fs_blocks << sb->s_blocksize_bits);
 	mark_inode_dirty(inode);
+	set_bit(HFSPLUS_I_ALLOC_DIRTY, &HFSPLUS_I(inode)->flags);
 }
Index: linux-2.6/fs/hfsplus/ioctl.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/ioctl.c	2010-11-17 22:43:05.452004821 +0100
+++ linux-2.6/fs/hfsplus/ioctl.c	2010-11-17 22:56:01.498003634 +0100
@@ -147,9 +147,11 @@ int hfsplus_setxattr(struct dentry *dent
 			res = -ERANGE;
 	} else
 		res = -EOPNOTSUPP;
-	if (!res)
+	if (!res) {
 		hfs_bnode_write(fd.bnode, &entry, fd.entryoffset,
 				sizeof(struct hfsplus_cat_file));
+		hfsplus_mark_catalog_dirty(inode);
+	}
 out:
 	hfs_find_exit(&fd);
 	return res;
Index: linux-2.6/fs/hfsplus/super.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/super.c	2010-11-17 22:54:53.845253878 +0100
+++ linux-2.6/fs/hfsplus/super.c	2010-11-17 22:56:01.505005590 +0100
@@ -472,7 +472,7 @@ static int hfsplus_fill_super(struct sup
 				   &str, sbi->hidden_dir);
 		mutex_unlock(&sbi->vh_mutex);
 
-		mark_inode_dirty(sbi->hidden_dir);
+		hfsplus_mark_catalog_dirty(sbi->hidden_dir);
 	}
 out:
 	unload_nls(sbi->nls);

[PATCH 11/11] hfsplus: flush disk caches in sync and fsync

[Christoph Hellwig]

Flush the disk cache in fsync and sync to make sure data actually is
on disk on completion of these system calls.  There is a nobarrier
mount option to disable this behaviour.  It's slightly misnamed now
that barrier actually are gone, but it matches the name used by all
major filesystems.

Signed-off-by: Christoph Hellwig <hch@tuxera.com>

Index: linux-2.6/fs/hfsplus/inode.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/inode.c	2010-11-17 22:56:01.488273225 +0100
+++ linux-2.6/fs/hfsplus/inode.c	2010-11-17 23:03:35.845003983 +0100
@@ -8,6 +8,7 @@
  * Inode handling routines
  */
 
+#include <linux/blkdev.h>
 #include <linux/mm.h>
 #include <linux/fs.h>
 #include <linux/pagemap.h>
@@ -337,6 +338,9 @@ int hfsplus_file_fsync(struct file *file
 			error = error2;
 	}
 
+	if (!test_bit(HFSPLUS_SB_NOBARRIER, &sbi->flags))
+		blkdev_issue_flush(inode->i_sb->s_bdev, GFP_KERNEL, NULL);
+
 	return error;
 }
 
Index: linux-2.6/fs/hfsplus/super.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/super.c	2010-11-17 22:56:01.505005590 +0100
+++ linux-2.6/fs/hfsplus/super.c	2010-11-17 23:03:43.735253739 +0100
@@ -10,6 +10,7 @@
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/pagemap.h>
+#include <linux/blkdev.h>
 #include <linux/fs.h>
 #include <linux/slab.h>
 #include <linux/vfs.h>
@@ -212,6 +213,10 @@ int hfsplus_sync_fs(struct super_block *
 out:
 	mutex_unlock(&sbi->alloc_mutex);
 	mutex_unlock(&sbi->vh_mutex);
+
+	if (!test_bit(HFSPLUS_SB_NOBARRIER, &sbi->flags))
+		blkdev_issue_flush(sb->s_bdev, GFP_KERNEL, NULL);
+
 	return error;
 }
 
Index: linux-2.6/fs/hfsplus/hfsplus_fs.h
===================================================================
--- linux-2.6.orig/fs/hfsplus/hfsplus_fs.h	2010-11-17 22:56:01.488273225 +0100
+++ linux-2.6/fs/hfsplus/hfsplus_fs.h	2010-11-17 22:57:48.130255066 +0100
@@ -156,6 +156,7 @@ struct hfsplus_sb_info {
 #define HFSPLUS_SB_FORCE	2
 #define HFSPLUS_SB_HFSX		3
 #define HFSPLUS_SB_CASEFOLD	4
+#define HFSPLUS_SB_NOBARRIER	5
 
 static inline struct hfsplus_sb_info *HFSPLUS_SB(struct super_block *sb)
 {
Index: linux-2.6/fs/hfsplus/options.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/options.c	2010-11-17 22:43:05.314004052 +0100
+++ linux-2.6/fs/hfsplus/options.c	2010-11-17 22:57:48.135254228 +0100
@@ -23,6 +23,7 @@ enum {
 	opt_umask, opt_uid, opt_gid,
 	opt_part, opt_session, opt_nls,
 	opt_nodecompose, opt_decompose,
+	opt_barrier, opt_nobarrier,
 	opt_force, opt_err
 };
 
@@ -37,6 +38,8 @@ static const match_table_t tokens = {
 	{ opt_nls, "nls=%s" },
 	{ opt_decompose, "decompose" },
 	{ opt_nodecompose, "nodecompose" },
+	{ opt_barrier, "barrier" },
+	{ opt_nobarrier, "nobarrier" },
 	{ opt_force, "force" },
 	{ opt_err, NULL }
 };
@@ -174,6 +177,12 @@ int hfsplus_parse_options(char *input, s
 		case opt_nodecompose:
 			set_bit(HFSPLUS_SB_NODECOMPOSE, &sbi->flags);
 			break;
+		case opt_barrier:
+			clear_bit(HFSPLUS_SB_NOBARRIER, &sbi->flags);
+			break;
+		case opt_nobarrier:
+			set_bit(HFSPLUS_SB_NOBARRIER, &sbi->flags);
+			break;
 		case opt_force:
 			set_bit(HFSPLUS_SB_FORCE, &sbi->flags);
 			break;
@@ -212,5 +221,7 @@ int hfsplus_show_options(struct seq_file
 		seq_printf(seq, ",nls=%s", sbi->nls->charset);
 	if (test_bit(HFSPLUS_SB_NODECOMPOSE, &sbi->flags))
 		seq_printf(seq, ",nodecompose");
+	if (test_bit(HFSPLUS_SB_NOBARRIER, &sbi->flags))
+		seq_printf(seq, ",nobarrier");
 	return 0;
 }

Re: [PATCH 10/11] hfsplus: optimize fsync

[Nick Piggin]

On Thu, Nov 18, 2010 at 9:23 AM, Christoph Hellwig <hch@lst.de> wrote:
> Avoid doing unessecary work in fsync.  Do nothing unless the inode
> was marked dirty, and only write the various metadata inodes out if
> they contain any dirty state from this inode.  This is archived by
> adding three new dirty bits to the hfsplus-specific inode which are
> set in the correct places.

Hmm, do you have some i_state coherency problems here?

>
>  int hfsplus_file_fsync(struct file *file, int datasync)
>  {
>        struct inode *inode = file->f_mapping->host;
> +       struct hfsplus_inode_info *hip = HFSPLUS_I(inode);
>        struct hfsplus_sb_info *sbi = HFSPLUS_SB(inode->i_sb);
> -       int error, error2;
> +       int error = 0, error2;
> +
> +       if (!(inode->i_state & I_DIRTY))
> +               return 0;

Does this guy ever get cleared in the synchronous path? Or only in writeback?
I couldn't see where it gets cleared from direct writeout, I wonder if we should
import the pagecache tagged dirty check from writeback to keep this in sync
better? (anyway, separate issue)

Anyway, what if writeback noticed pagecache was cleaned at this point, and then
clears I_DIRTY bits from inode before you test it above? Won't that leave your
metadata not on disk?

>
>        /*
>         * Sync inode metadata into the catalog and extent trees.
> @@ -318,13 +322,21 @@ int hfsplus_file_fsync(struct file *file
>        /*
>         * And explicitly write out the btrees.
>         */
> -       error = filemap_write_and_wait(sbi->cat_tree->inode->i_mapping);
> -       error2 = filemap_write_and_wait(sbi->ext_tree->inode->i_mapping);
> -       if (!error)
> -               error = error2;
> -       error2 = filemap_write_and_wait(sbi->alloc_file->i_mapping);
> -       if (!error)
> -               error = error2;
> +       if (test_and_clear_bit(HFSPLUS_I_CAT_DIRTY, &hip->flags))
> +               error = filemap_write_and_wait(sbi->cat_tree->inode->i_mapping);
> +
> +       if (test_and_clear_bit(HFSPLUS_I_EXT_DIRTY, &hip->flags)) {
> +               error2 = filemap_write_and_wait(sbi->ext_tree->inode->i_mapping);
> +               if (!error)
> +                       error = error2;
> +       }
> +
> +       if (test_and_clear_bit(HFSPLUS_I_ALLOC_DIRTY, &hip->flags)) {
> +               error2 = filemap_write_and_wait(sbi->alloc_file->i_mapping);
> +               if (!error)
> +                       error = error2;
> +       }
> +
>        return error;
>  }
>
> @@ -590,6 +602,8 @@ int hfsplus_cat_write_inode(struct inode
>                hfs_bnode_write(fd.bnode, &entry, fd.entryoffset,
>                                         sizeof(struct hfsplus_cat_file));
>        }
> +
> +       set_bit(HFSPLUS_I_CAT_DIRTY, &HFSPLUS_I(inode)->flags);
>  out:
>        hfs_find_exit(&fd);
>        return 0;

So your I_CAT_DIRTY will be set, but not I_DIRTY, AFAIKS.


> @@ -530,4 +532,5 @@ out:
>        hip->fs_blocks = (inode->i_size + sb->s_blocksize - 1) >> sb->s_blocksize_bits;
>        inode_set_bytes(inode, hip->fs_blocks << sb->s_blocksize_bits);
>        mark_inode_dirty(inode);
> +       set_bit(HFSPLUS_I_ALLOC_DIRTY, &HFSPLUS_I(inode)->flags);

If you mark these sub-parts of the inode dirty after marking the inode
dirty, and no
locking, then AFAIKS you get no guarantee they will be noticed at the point that
I_DIRTY bits are cleared.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 10/11] hfsplus: optimize fsync

[Christoph Hellwig]

On Thu, Nov 18, 2010 at 05:40:44PM +1100, Nick Piggin wrote:
> Does this guy ever get cleared in the synchronous path? Or only in writeback?
> I couldn't see where it gets cleared from direct writeout, I wonder if we should
> import the pagecache tagged dirty check from writeback to keep this in sync
> better? (anyway, separate issue)
> 
> Anyway, what if writeback noticed pagecache was cleaned at this point, and then
> clears I_DIRTY bits from inode before you test it above? Won't that leave your
> metadata not on disk?

What do you mean with direct writeout?  For hfsplus we always write
the inode through writeback_single_inode, either from the writeback
threads, or through sync_inode_metadata (or previously write_inode_now)
in fsync.  Both of these clear I_DIRTY.  But one thing I noticed when
reading through the above is that I_DIRTY is overkill here - we only
care about metadata, as the pagecache is handled by vfs_fsync. Just
checking I_DIRTY_SYNC (and eventually I_DIRTY_DATASYNC when adding
support for an optimized fdatasync) would be enough.  The code is
take from generic_file_fsync, which is used or copied by a lot of
filesystems, so it looks like no one really cared about the possibly
superflous I_DIRTY_PAGES checking so far.

> > @@ -590,6 +602,8 @@ int hfsplus_cat_write_inode(struct inode
> > ? ? ? ? ? ? ? ?hfs_bnode_write(fd.bnode, &entry, fd.entryoffset,
> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? sizeof(struct hfsplus_cat_file));
> > ? ? ? ?}
> > +
> > + ? ? ? set_bit(HFSPLUS_I_CAT_DIRTY, &HFSPLUS_I(inode)->flags);
> > ?out:
> > ? ? ? ?hfs_find_exit(&fd);
> > ? ? ? ?return 0;
> 
> So your I_CAT_DIRTY will be set, but not I_DIRTY, AFAIKS.

This in ->write_inode which is called to clear I_DIRTY (well,
(I_DIRTY_SYNC|I_DIRTY_DATASYNC).  But given the way how hfsplus stores
it's equivalents of inode and dirent in the catalog btree it means
dirtying the btree.  If ->write_inode is called by fsync that gets
picked up by the fsync code checking for I_CAT_DIRTY, if it's for
sync or during umount it's caught by the unconditionaly flush of it
in hfsplus_sync_fs.  The write_inode during nfs exporting is not handled
yet, but adding a commit_metadata operation is on my todo list.

> > @@ -530,4 +532,5 @@ out:
> > ? ? ? ?hip->fs_blocks = (inode->i_size + sb->s_blocksize - 1) >> sb->s_blocksize_bits;
> > ? ? ? ?inode_set_bytes(inode, hip->fs_blocks << sb->s_blocksize_bits);
> > ? ? ? ?mark_inode_dirty(inode);
> > + ? ? ? set_bit(HFSPLUS_I_ALLOC_DIRTY, &HFSPLUS_I(inode)->flags);
> 
> If you mark these sub-parts of the inode dirty after marking the inode
> dirty, and no
> locking, then AFAIKS you get no guarantee they will be noticed at the point that
> I_DIRTY bits are cleared.

Yes, it should be before.  I had fixed that up in the other places but
missed this one.  Thanks for the review!

Re: [PATCH 10/11] hfsplus: optimize fsync

[Nick Piggin]

On Fri, Nov 19, 2010 at 12:50 AM, Christoph Hellwig <hch@lst.de> wrote:
> On Thu, Nov 18, 2010 at 05:40:44PM +1100, Nick Piggin wrote:
>> Does this guy ever get cleared in the synchronous path? Or only in writeback?
>> I couldn't see where it gets cleared from direct writeout, I wonder if we should
>> import the pagecache tagged dirty check from writeback to keep this in sync
>> better? (anyway, separate issue)

[cut]

>
> What do you mean with direct writeout?

Bad terminology, I meant actually fsync driven writeout.


>  For hfsplus we always write
> the inode through writeback_single_inode, either from the writeback
> threads, or through sync_inode_metadata (or previously write_inode_now)
> in fsync.  Both of these clear I_DIRTY.

Ah OK, I missed that one, thanks.


>  But one thing I noticed when
> reading through the above is that I_DIRTY is overkill here - we only
> care about metadata, as the pagecache is handled by vfs_fsync. Just
> checking I_DIRTY_SYNC (and eventually I_DIRTY_DATASYNC when adding
> support for an optimized fdatasync) would be enough.

Yes that's true.


>  The code is
> take from generic_file_fsync, which is used or copied by a lot of
> filesystems, so it looks like no one really cared about the possibly
> superflous I_DIRTY_PAGES checking so far.

[paste]
>>
>> Anyway, what if writeback noticed pagecache was cleaned at this point, and then
>> clears I_DIRTY bits from inode before you test it above? Won't that leave your
>> metadata not on disk?

I'm not sure if you answered this, though. Can't background writeout go through
and clear the I_DIRTY_* bits on your inode, so that a subsequent fsync will skip
required writeout because i_state is clean, but your private dirty
bits are still set?

That was my (poorly worded) concern.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 10/11] hfsplus: optimize fsync

[Christoph Hellwig]

On Fri, Nov 19, 2010 at 01:13:55AM +1100, Nick Piggin wrote:
> >> Anyway, what if writeback noticed pagecache was cleaned at this point, and then
> >> clears I_DIRTY bits from inode before you test it above? Won't that leave your
> >> metadata not on disk?
> 
> I'm not sure if you answered this, though. Can't background writeout go through
> and clear the I_DIRTY_* bits on your inode, so that a subsequent fsync will skip
> required writeout because i_state is clean, but your private dirty
> bits are still set?
> 
> That was my (poorly worded) concern.

Good point - we should just skip the sync_inode_metadata for that case.
Which already is a no-op if no dirty bits are set, so just removing the
I_DIRTY check probably is the best thing we can do here.

[PATCH v2] hfsplus: optimize fsync

[Christoph Hellwig]

Avoid doing unessecary work in fsync.  Do nothing unless the inode
was marked dirty, and only write the various metadata inodes out if
they contain any dirty state from this inode.  This is archived by
adding three new dirty bits to the hfsplus-specific inode which are
set in the correct places.

Signed-off-by: Christoph Hellwig <hch@tuxera.com>

Index: linux-2.6/fs/hfsplus/inode.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/inode.c	2010-11-19 15:59:12.449011456 +0100
+++ linux-2.6/fs/hfsplus/inode.c	2010-11-19 16:00:57.584004193 +0100
@@ -307,8 +307,9 @@ static int hfsplus_setattr(struct dentry
 int hfsplus_file_fsync(struct file *file, int datasync)
 {
 	struct inode *inode = file->f_mapping->host;
+	struct hfsplus_inode_info *hip = HFSPLUS_I(inode);
 	struct hfsplus_sb_info *sbi = HFSPLUS_SB(inode->i_sb);
-	int error, error2;
+	int error = 0, error2;
 
 	/*
 	 * Sync inode metadata into the catalog and extent trees.
@@ -318,13 +319,21 @@ int hfsplus_file_fsync(struct file *file
 	/*
 	 * And explicitly write out the btrees.
 	 */
-	error = filemap_write_and_wait(sbi->cat_tree->inode->i_mapping);
-	error2 = filemap_write_and_wait(sbi->ext_tree->inode->i_mapping);
-	if (!error)
-		error = error2;
-	error2 = filemap_write_and_wait(sbi->alloc_file->i_mapping);
-	if (!error)
-		error = error2;
+	if (test_and_clear_bit(HFSPLUS_I_CAT_DIRTY, &hip->flags))
+		error = filemap_write_and_wait(sbi->cat_tree->inode->i_mapping);
+
+	if (test_and_clear_bit(HFSPLUS_I_EXT_DIRTY, &hip->flags)) {
+		error2 = filemap_write_and_wait(sbi->ext_tree->inode->i_mapping);
+		if (!error)
+			error = error2;
+	}
+
+	if (test_and_clear_bit(HFSPLUS_I_ALLOC_DIRTY, &hip->flags)) {
+		error2 = filemap_write_and_wait(sbi->alloc_file->i_mapping);
+		if (!error)
+			error = error2;
+	}
+
 	return error;
 }
 
@@ -590,6 +599,8 @@ int hfsplus_cat_write_inode(struct inode
 		hfs_bnode_write(fd.bnode, &entry, fd.entryoffset,
 					 sizeof(struct hfsplus_cat_file));
 	}
+
+	set_bit(HFSPLUS_I_CAT_DIRTY, &HFSPLUS_I(inode)->flags);
 out:
 	hfs_find_exit(&fd);
 	return 0;
Index: linux-2.6/fs/hfsplus/hfsplus_fs.h
===================================================================
--- linux-2.6.orig/fs/hfsplus/hfsplus_fs.h	2010-11-19 15:59:12.451026472 +0100
+++ linux-2.6/fs/hfsplus/hfsplus_fs.h	2010-11-19 16:16:18.271003843 +0100
@@ -157,6 +157,11 @@ struct hfsplus_sb_info {
 #define HFSPLUS_SB_HFSX		3
 #define HFSPLUS_SB_CASEFOLD	4
 
+static inline struct hfsplus_sb_info *HFSPLUS_SB(struct super_block *sb)
+{
+	return sb->s_fs_info;
+}
+
 
 struct hfsplus_inode_info {
 	atomic_t opencnt;
@@ -205,10 +210,31 @@ struct hfsplus_inode_info {
 #define HFSPLUS_EXT_NEW		0x0002
 
 #define HFSPLUS_I_RSRC		0	/* represents a resource fork */
+#define HFSPLUS_I_CAT_DIRTY	1	/* has changes in the catalog tree */
+#define HFSPLUS_I_EXT_DIRTY	2	/* has changes in the extent tree */
+#define HFSPLUS_I_ALLOC_DIRTY	3	/* has changes in the allocation file */
 
 #define HFSPLUS_IS_RSRC(inode) \
 	test_bit(HFSPLUS_I_RSRC, &HFSPLUS_I(inode)->flags)
 
+static inline struct hfsplus_inode_info *HFSPLUS_I(struct inode *inode)
+{
+	return list_entry(inode, struct hfsplus_inode_info, vfs_inode);
+}
+
+/*
+ * Mark an inode dirty, and also mark the btree in which the
+ * specific type of metadata is stored.
+ * For data or metadata that gets written back by into the catalog btree
+ * by hfsplus_write_inode a plain mark_inode_dirty call is enough.
+ */
+static inline void hfsplus_mark_inode_dirty(struct inode *inode,
+		unsigned int flag)
+{
+	set_bit(flag, &HFSPLUS_I(inode)->flags);
+	mark_inode_dirty(inode);
+}
+
 struct hfs_find_data {
 	/* filled by caller */
 	hfsplus_btree_key *search_key;
@@ -396,17 +422,6 @@ int hfs_part_find(struct super_block *,
 int hfsplus_submit_bio(struct block_device *bdev, sector_t sector,
 		void *data, int rw);
 
-/* access macros */
-static inline struct hfsplus_sb_info *HFSPLUS_SB(struct super_block *sb)
-{
-	return sb->s_fs_info;
-}
-
-static inline struct hfsplus_inode_info *HFSPLUS_I(struct inode *inode)
-{
-	return list_entry(inode, struct hfsplus_inode_info, vfs_inode);
-}
-
 /* time macros */
 #define __hfsp_mt2ut(t)		(be32_to_cpu(t) - 2082844800U)
 #define __hfsp_ut2mt(t)		(cpu_to_be32(t + 2082844800U))
Index: linux-2.6/fs/hfsplus/catalog.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/catalog.c	2010-11-17 23:29:28.746254298 +0100
+++ linux-2.6/fs/hfsplus/catalog.c	2010-11-19 16:13:52.691004053 +0100
@@ -227,7 +227,8 @@ int hfsplus_create_cat(u32 cnid, struct
 
 	dir->i_size++;
 	dir->i_mtime = dir->i_ctime = CURRENT_TIME_SEC;
-	mark_inode_dirty(dir);
+	hfsplus_mark_inode_dirty(dir, HFSPLUS_I_CAT_DIRTY);
+
 	hfs_find_exit(&fd);
 	return 0;
 
@@ -308,7 +309,7 @@ int hfsplus_delete_cat(u32 cnid, struct
 
 	dir->i_size--;
 	dir->i_mtime = dir->i_ctime = CURRENT_TIME_SEC;
-	mark_inode_dirty(dir);
+	hfsplus_mark_inode_dirty(dir, HFSPLUS_I_CAT_DIRTY);
 out:
 	hfs_find_exit(&fd);
 
@@ -353,7 +354,6 @@ int hfsplus_rename_cat(u32 cnid,
 		goto out;
 	dst_dir->i_size++;
 	dst_dir->i_mtime = dst_dir->i_ctime = CURRENT_TIME_SEC;
-	mark_inode_dirty(dst_dir);
 
 	/* finally remove the old entry */
 	hfsplus_cat_build_key(sb, src_fd.search_key, src_dir->i_ino, src_name);
@@ -365,7 +365,6 @@ int hfsplus_rename_cat(u32 cnid,
 		goto out;
 	src_dir->i_size--;
 	src_dir->i_mtime = src_dir->i_ctime = CURRENT_TIME_SEC;
-	mark_inode_dirty(src_dir);
 
 	/* remove old thread entry */
 	hfsplus_cat_build_key(sb, src_fd.search_key, cnid, NULL);
@@ -387,6 +386,9 @@ int hfsplus_rename_cat(u32 cnid,
 		goto out;
 	}
 	err = hfs_brec_insert(&dst_fd, &entry, entry_size);
+
+	hfsplus_mark_inode_dirty(dst_dir, HFSPLUS_I_CAT_DIRTY);
+	hfsplus_mark_inode_dirty(src_dir, HFSPLUS_I_CAT_DIRTY);
 out:
 	hfs_bnode_put(dst_fd.bnode);
 	hfs_find_exit(&src_fd);
Index: linux-2.6/fs/hfsplus/extents.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/extents.c	2010-11-19 15:59:12.459278532 +0100
+++ linux-2.6/fs/hfsplus/extents.c	2010-11-19 16:26:15.549011667 +0100
@@ -108,6 +108,14 @@ static void __hfsplus_ext_write_extent(s
 				fd->entryoffset, fd->entrylength);
 		hip->extent_state &= ~HFSPLUS_EXT_DIRTY;
 	}
+
+	/*
+	 * We can't just use hfsplus_mark_inode_dirty here, because we
+	 * also get called from hfsplus_write_inode, which should not
+	 * redirty the inode.  Instead the callers have to be careful
+	 * to explicily mark the inode dirty, too.
+	 */
+	set_bit(HFSPLUS_I_EXT_DIRTY, &hip->flags);
 }
 
 static void hfsplus_ext_write_extent_locked(struct inode *inode)
@@ -197,6 +205,7 @@ int hfsplus_get_block(struct inode *inod
 	struct hfsplus_inode_info *hip = HFSPLUS_I(inode);
 	int res = -EIO;
 	u32 ablock, dblock, mask;
+	int was_dirty = 0;
 	int shift;
 
 	/* Convert inode block to disk allocation block */
@@ -223,14 +232,20 @@ int hfsplus_get_block(struct inode *inod
 		return -EIO;
 
 	mutex_lock(&hip->extents_lock);
+
+	/*
+	 * hfsplus_ext_read_extent will write out a cached extent into
+	 * the extents btree.  In that case we may have to mark the inode
+	 * dirty even for a pure read of an extent here.
+	 */
+	was_dirty = (hip->extent_state & HFSPLUS_EXT_DIRTY);
 	res = hfsplus_ext_read_extent(inode, ablock);
-	if (!res) {
-		dblock = hfsplus_ext_find_block(hip->cached_extents,
-						ablock - hip->cached_start);
-	} else {
+	if (res) {
 		mutex_unlock(&hip->extents_lock);
 		return -EIO;
 	}
+	dblock = hfsplus_ext_find_block(hip->cached_extents,
+					ablock - hip->cached_start);
 	mutex_unlock(&hip->extents_lock);
 
 done:
@@ -242,8 +257,9 @@ done:
 		hip->phys_size += sb->s_blocksize;
 		hip->fs_blocks++;
 		inode_add_bytes(inode, sb->s_blocksize);
-		mark_inode_dirty(inode);
 	}
+	if (create || was_dirty)
+		mark_inode_dirty(inode);
 	return 0;
 }
 
@@ -438,7 +454,7 @@ out:
 	mutex_unlock(&hip->extents_lock);
 	if (!res) {
 		hip->alloc_blocks += len;
-		mark_inode_dirty(inode);
+		hfsplus_mark_inode_dirty(inode, HFSPLUS_I_ALLOC_DIRTY);
 	}
 	return res;
 
@@ -529,5 +545,5 @@ out:
 	hip->phys_size = inode->i_size;
 	hip->fs_blocks = (inode->i_size + sb->s_blocksize - 1) >> sb->s_blocksize_bits;
 	inode_set_bytes(inode, hip->fs_blocks << sb->s_blocksize_bits);
-	mark_inode_dirty(inode);
+	hfsplus_mark_inode_dirty(inode, HFSPLUS_I_ALLOC_DIRTY);
 }
Index: linux-2.6/fs/hfsplus/ioctl.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/ioctl.c	2010-11-17 23:29:28.766253320 +0100
+++ linux-2.6/fs/hfsplus/ioctl.c	2010-11-19 16:14:23.369004333 +0100
@@ -147,9 +147,11 @@ int hfsplus_setxattr(struct dentry *dent
 			res = -ERANGE;
 	} else
 		res = -EOPNOTSUPP;
-	if (!res)
+	if (!res) {
 		hfs_bnode_write(fd.bnode, &entry, fd.entryoffset,
 				sizeof(struct hfsplus_cat_file));
+		hfsplus_mark_inode_dirty(inode, HFSPLUS_I_CAT_DIRTY);
+	}
 out:
 	hfs_find_exit(&fd);
 	return res;
Index: linux-2.6/fs/hfsplus/super.c
===================================================================
--- linux-2.6.orig/fs/hfsplus/super.c	2010-11-19 15:59:12.457024656 +0100
+++ linux-2.6/fs/hfsplus/super.c	2010-11-19 16:14:09.930004192 +0100
@@ -474,7 +474,7 @@ static int hfsplus_fill_super(struct sup
 				   &str, sbi->hidden_dir);
 		mutex_unlock(&sbi->vh_mutex);
 
-		mark_inode_dirty(sbi->hidden_dir);
+		hfsplus_mark_inode_dirty(sbi->hidden_dir, HFSPLUS_I_CAT_DIRTY);
 	}
 out:
 	unload_nls(sbi->nls);

Re: [PATCH 10/11] hfsplus: optimize fsync

[Nick Piggin]

On Thu, Nov 18, 2010 at 03:16:57PM +0100, Christoph Hellwig wrote:
> On Fri, Nov 19, 2010 at 01:13:55AM +1100, Nick Piggin wrote:
> > >> Anyway, what if writeback noticed pagecache was cleaned at this point, and then
> > >> clears I_DIRTY bits from inode before you test it above? Won't that leave your
> > >> metadata not on disk?
> > 
> > I'm not sure if you answered this, though. Can't background writeout go through
> > and clear the I_DIRTY_* bits on your inode, so that a subsequent fsync will skip
> > required writeout because i_state is clean, but your private dirty
> > bits are still set?
> > 
> > That was my (poorly worded) concern.
> 
> Good point - we should just skip the sync_inode_metadata for that case.
> Which already is a no-op if no dirty bits are set, so just removing the
> I_DIRTY check probably is the best thing we can do here.

Hmm, it seems that your dirty bit handling is still broken and racy,
along with half the other filesystems. I'll post a couple of patches to
fsdevel for comment.

Re: [PATCH 10/11] hfsplus: optimize fsync

[Nick Piggin]

On Tue, Nov 23, 2010 at 12:03:14AM +1100, Nick Piggin wrote:
> On Thu, Nov 18, 2010 at 03:16:57PM +0100, Christoph Hellwig wrote:
> > On Fri, Nov 19, 2010 at 01:13:55AM +1100, Nick Piggin wrote:
> > > >> Anyway, what if writeback noticed pagecache was cleaned at this point, and then
> > > >> clears I_DIRTY bits from inode before you test it above? Won't that leave your
> > > >> metadata not on disk?
> > > 
> > > I'm not sure if you answered this, though. Can't background writeout go through
> > > and clear the I_DIRTY_* bits on your inode, so that a subsequent fsync will skip
> > > required writeout because i_state is clean, but your private dirty
> > > bits are still set?
> > > 
> > > That was my (poorly worded) concern.
> > 
> > Good point - we should just skip the sync_inode_metadata for that case.
> > Which already is a no-op if no dirty bits are set, so just removing the
> > I_DIRTY check probably is the best thing we can do here.
> 
> Hmm, it seems that your dirty bit handling is still broken and racy,
> along with half the other filesystems. I'll post a couple of patches to
> fsdevel for comment.

Actually now I look at your updated patch, perhaps it is not. What
version does that patch apply against? Do you have a tree or tarball
uploaded anywhere? Or can you repost the full series?

Thanks,
Nick

Re: [PATCH 10/11] hfsplus: optimize fsync

[Christoph Hellwig]

On Tue, Nov 23, 2010 at 12:18:08AM +1100, Nick Piggin wrote:
> Actually now I look at your updated patch, perhaps it is not. What
> version does that patch apply against? Do you have a tree or tarball
> uploaded anywhere? Or can you repost the full series?

It applies instead of the previous patch in that series, that is after
the 9 patches before it.  I don't think hfsplus has issue you mentioned
in that other thread, not by design but more by accident due to:

 - not checking the dirty bits anymore as it doesn't implement the
   fdatasync optimization.
 - hfsplus_write_inode first calling hfsplus_ext_write_extent before
   setting the catalog dirty bit, with hfsplus_ext_write_extent always
   locking and unlocking a mutex and thus providing the required memory
   barrier.