ufs: rellocation fix

PATCH In review Evgeniy Dushistov 3 participants 4 messages Jan 22, 2007 – Jan 25, 2007 VFS

Share & tools

Evgeniy Dushistov

author

2007-01-22 15:02 UTC #0 lore raw

PATCH 3/3 ufs: rellocation fix

In blocks reallocation function sometimes does not update some of buffer_head::b_blocknr, which may and cause data damage.

Signed-off-by Evgeniy Dushistov <[email protected]>

---

fs/ufs/balloc.c +24 ~~-13~~

Index: linux-2.6.20-rc5/fs/ufs/balloc.c
===================================================================
@@ -231,10 +231,10 @@ static void ufs_change_blocknr(struct in
 			       unsigned int count, unsigned int oldb,
 			       unsigned int newb, struct page *locked_page)
 {
-	unsigned int blk_per_page = 1 << (PAGE_CACHE_SHIFT - inode->i_blkbits);
-	struct address_space *mapping = inode->i_mapping;
+	const unsigned mask = (1 << (PAGE_CACHE_SHIFT - inode->i_blkbits)) - 1;
+	struct address_space * const mapping = inode->i_mapping;
 	pgoff_t index, cur_index;
-	unsigned int i, j;
+	unsigned i, pos, j;
 	struct page *page;
 	struct buffer_head *head, *bh;

@@ -246,7 +246,7 @@ static void ufs_change_blocknr(struct in
 
 	cur_index = locked_page->index;
 
-	for (i = 0; i < count; i += blk_per_page) {
+	for (i = 0; i < count; i = (i | mask) + 1) {
 		index = (baseblk+i) >> (PAGE_CACHE_SHIFT - inode->i_blkbits);
 
 		if (likely(cur_index != index)) {

@@ -256,21 +256,32 @@ static void ufs_change_blocknr(struct in
 		} else
 			page = locked_page;
 
-		j = i;
 		head = page_buffers(page);
 		bh = head;
+		pos = i & mask;
+		for (j = 0; j < pos; ++j)
+			bh = bh->b_this_page;
+		j = 0;
 		do {
-			if (likely(bh->b_blocknr == j + oldb && j < count)) {
-				unmap_underlying_metadata(bh->b_bdev,
-							  bh->b_blocknr);
-				bh->b_blocknr = newb + j++;
-				mark_buffer_dirty(bh);
+			if (buffer_mapped(bh)) {
+				pos = bh->b_blocknr - oldb;
+				if (pos < count) {
+					UFSD(" change from %llu to %llu\n",
+					     (unsigned long long)pos + odlb,
+					     (unsigned long long)pos + newb);
+					bh->b_blocknr = newb + pos;
+					unmap_underlying_metadata(bh->b_bdev,
+								  bh->b_blocknr);
+					mark_buffer_dirty(bh);
+					++j;
+				}
 			}
 
 			bh = bh->b_this_page;
 		} while (bh != head);
 
-		set_page_dirty(page);
+		if (j)
+			set_page_dirty(page);
 
 		if (likely(cur_index != index))
 			ufs_put_locked_page(page);

@@ -418,14 +429,14 @@ unsigned ufs_new_fragments(struct inode 
 	}
 	result = ufs_alloc_fragments (inode, cgno, goal, request, err);
 	if (result) {
+		ufs_clear_frags(inode, result + oldcount, newcount - oldcount,
+				locked_page != NULL);
 		ufs_change_blocknr(inode, fragment - oldcount, oldcount, tmp,
 				   result, locked_page);
 
 		*p = cpu_to_fs32(sb, result);
 		*err = 0;
 		UFS_I(inode)->i_lastfrag = max_t(u32, UFS_I(inode)->i_lastfrag, fragment + count);
-		ufs_clear_frags(inode, result + oldcount, newcount - oldcount,
-				locked_page != NULL);
 		unlock_super(sb);
 		if (newcount < request)
 			ufs_free_fragments (inode, result + newcount, request - newcount);

Signature · 1 line

/Evgeniy

Andrew Morton

2007-01-25 04:21 UTC #1 lore raw

Quoted 26 lines · On Mon, 22 Jan 2007 18:07:51 +0300 Evgeniy Dushistov <[email protected]> wrote:

In blocks reallocation function sometimes does not update some of buffer_head::b_blocknr, which may and cause data damage.

Signed-off-by Evgeniy Dushistov <[email protected]>

---

Index: linux-2.6.20-rc5/fs/ufs/balloc.c
===================================================================

fs/ufs/balloc.c +3 -3

@@ -231,10 +231,10 @@ static void ufs_change_blocknr(struct in
 			       unsigned int count, unsigned int oldb,
 			       unsigned int newb, struct page *locked_page)
 {
-	unsigned int blk_per_page = 1 << (PAGE_CACHE_SHIFT - inode->i_blkbits);
-	struct address_space *mapping = inode->i_mapping;
+	const unsigned mask = (1 << (PAGE_CACHE_SHIFT - inode->i_blkbits)) - 1;
+	struct address_space * const mapping = inode->i_mapping;
 	pgoff_t index, cur_index;
-	unsigned int i, j;
+	unsigned i, pos, j;
 	struct page *page;
 	struct buffer_head *head, *bh;

fs/ufs/balloc.c static void ufs_change_blocknr(struct in · line 249

Quoted 5 lines

fs/ufs/balloc.c +1 -1

@@ -246,7 +246,7 @@ static void ufs_change_blocknr(struct in
 
 	cur_index = locked_page->index;
 
-	for (i = 0; i < count; i += blk_per_page) {
+	for (i = 0; i < count; i = (i | mask) + 1) {

This is a funny looking thing. As far as I can tell, this is a more complicated way of doing the same thing as the old code did.

Am I missing something?

fs/ufs/balloc.c static void ufs_change_blocknr(struct in · line 261

Quoted 11 lines

fs/ufs/balloc.c +1 -1

 		index = (baseblk+i) >> (PAGE_CACHE_SHIFT - inode->i_blkbits);
 
 		if (likely(cur_index != index)) {

@@ -256,21 +256,32 @@ static void ufs_change_blocknr(struct in
 		} else
 			page = locked_page;
 
-		j = i;
 		head = page_buffers(page);
 		bh = head;
+		pos = i & mask;

And this is equivalent to

		pos = 0;

Quoted 51 lines

fs/ufs/balloc.c +19 -8

+		for (j = 0; j < pos; ++j)
+			bh = bh->b_this_page;
+		j = 0;
 		do {
-			if (likely(bh->b_blocknr == j + oldb && j < count)) {
-				unmap_underlying_metadata(bh->b_bdev,
-							  bh->b_blocknr);
-				bh->b_blocknr = newb + j++;
-				mark_buffer_dirty(bh);
+			if (buffer_mapped(bh)) {
+				pos = bh->b_blocknr - oldb;
+				if (pos < count) {
+					UFSD(" change from %llu to %llu\n",
+					     (unsigned long long)pos + odlb,
+					     (unsigned long long)pos + newb);
+					bh->b_blocknr = newb + pos;
+					unmap_underlying_metadata(bh->b_bdev,
+								  bh->b_blocknr);
+					mark_buffer_dirty(bh);
+					++j;
+				}
 			}
 
 			bh = bh->b_this_page;
 		} while (bh != head);
 
-		set_page_dirty(page);
+		if (j)
+			set_page_dirty(page);
 
 		if (likely(cur_index != index))
 			ufs_put_locked_page(page);

@@ -418,14 +429,14 @@ unsigned ufs_new_fragments(struct inode 
 	}
 	result = ufs_alloc_fragments (inode, cgno, goal, request, err);
 	if (result) {
+		ufs_clear_frags(inode, result + oldcount, newcount - oldcount,
+				locked_page != NULL);
 		ufs_change_blocknr(inode, fragment - oldcount, oldcount, tmp,
 				   result, locked_page);
 
 		*p = cpu_to_fs32(sb, result);
 		*err = 0;
 		UFS_I(inode)->i_lastfrag = max_t(u32, UFS_I(inode)->i_lastfrag, fragment + count);
-		ufs_clear_frags(inode, result + oldcount, newcount - oldcount,
-				locked_page != NULL);
 		unlock_super(sb);
 		if (newcount < request)
 			ufs_free_fragments (inode, result + newcount, request - newcount);

Signature · 1 line

/Evgeniy

张军伟

2007-01-25 08:54 UTC #2 lore raw

A question about ext3 journal

Hi all

	I think there is a problem about journal in ext3.
	in linux-2.6.17/include/linux/ext3_jbd.h

/* Define the number of blocks we need to account to a transaction to
* modify one block of data.
*
* We may have to touch one inode, one bitmap buffer, up to three
* indirection blocks, the group and superblock summaries, and the data
* block to complete the transaction. */

#define EXT3_SINGLEDATA_TRANS_BLOCKS 8U
The indirection blocks' modification also need their bitmap buffer and the group summaries be modified, I think. So the block count should be 8U + 3*2;

Additionally, the journal operations in namei.c or inode.c also have the problem like that.

Am I right? or misunderstand?

cheers, zhangjw

Evgeniy Dushistov

author

2007-01-25 10:40 UTC #3 lore raw

In reply to Andrew Morton

Quoted 11 lines · Andrew Morton

Review
Quoted 2 lines · On Mon, 22 Jan 2007 18:07:51 +0300 Evgeniy Dushistov <[email protected]> wrote:
(quoted diff) +1 -1
-	for (i = 0; i < count; i += blk_per_page) {
+	for (i = 0; i < count; i = (i | mask) + 1) {
This is a funny looking thing. As far as I can tell, this is a more complicated way of doing the same thing as the old code did.
Am I missing something?

No, you right, thanks, work should be done with baseblk+i, not with i. (It is imposible to catch on my test enviroment: size of page 4096 and baseblk always point to begin of data block which size of

Quoted 1 line

=4096 and power of 2).

Here is patch for patch, it also contains typo fixing in previous patch which lead to compilation failure if ufs debug turn on.

May be it is possible to integrate this patch into previous one, or resend new version of last patch?

---

fs/ufs/balloc.c +6 -6

Index: linux-2.6.20-rc5/fs/ufs/balloc.c
===================================================================
@@ -227,14 +227,14 @@ failed:
  * We can come here from ufs_writepage or ufs_prepare_write,
  * locked_page is argument of these functions, so we already lock it.
  */
-static void ufs_change_blocknr(struct inode *inode, unsigned int baseblk,
+static void ufs_change_blocknr(struct inode *inode, unsigned int beg,
 			       unsigned int count, unsigned int oldb,
 			       unsigned int newb, struct page *locked_page)
 {
 	const unsigned mask = (1 << (PAGE_CACHE_SHIFT - inode->i_blkbits)) - 1;
 	struct address_space * const mapping = inode->i_mapping;
 	pgoff_t index, cur_index;
-	unsigned i, pos, j;
+	unsigned end, pos, j;
 	struct page *page;
 	struct buffer_head *head, *bh;

@@ -246,8 +246,8 @@ static void ufs_change_blocknr(struct in
 
 	cur_index = locked_page->index;
 
-	for (i = 0; i < count; i = (i | mask) + 1) {
-		index = (baseblk+i) >> (PAGE_CACHE_SHIFT - inode->i_blkbits);
+	for (end = count + beg; beg < end; beg = (beg | mask) + 1) {
+		index = beg >> (PAGE_CACHE_SHIFT - inode->i_blkbits);
 
 		if (likely(cur_index != index)) {
 			page = ufs_get_locked_page(mapping, index);

@@ -258,7 +258,7 @@ static void ufs_change_blocknr(struct in
 
 		head = page_buffers(page);
 		bh = head;
-		pos = i & mask;
+		pos = beg & mask;
 		for (j = 0; j < pos; ++j)
 			bh = bh->b_this_page;
 		j = 0;

@@ -267,7 +267,7 @@ static void ufs_change_blocknr(struct in
 				pos = bh->b_blocknr - oldb;
 				if (pos < count) {
 					UFSD(" change from %llu to %llu\n",
-					     (unsigned long long)pos + odlb,
+					     (unsigned long long)pos + oldb,
 					     (unsigned long long)pos + newb);
 					bh->b_blocknr = newb + pos;
 					unmap_underlying_metadata(bh->b_bdev,

Signature · 1 line

/Evgeniy