[PATCH 3/3] ufs: rellocation fix

[PATCH 3/3] ufs: rellocation fix

[Evgeniy Dushistov]

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 <dushistov@mail.ru>

---

Index: linux-2.6.20-rc5/fs/ufs/balloc.c
===================================================================
--- linux-2.6.20-rc5.orig/fs/ufs/balloc.c
+++ 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);
-- 
/Evgeniy

Re: [PATCH 3/3] ufs: rellocation fix

[Andrew Morton]

On Mon, 22 Jan 2007 18:07:51 +0300
Evgeniy Dushistov <dushistov@mail.ru> 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 <dushistov@mail.ru>
> 
> ---
> 
> Index: linux-2.6.20-rc5/fs/ufs/balloc.c
> ===================================================================
> --- linux-2.6.20-rc5.orig/fs/ufs/balloc.c
> +++ 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) {

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?


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

?

> +		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);
> -- 
> /Evgeniy

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

Re: [PATCH 3/3] ufs: rellocation fix

[Evgeniy Dushistov]

On Wed, Jan 24, 2007 at 08:21:07PM -0800, Andrew Morton wrote:
> On Mon, 22 Jan 2007 18:07:51 +0300
> Evgeniy Dushistov <dushistov@mail.ru> wrote:
> 
> > -	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 
>=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?

---

Index: linux-2.6.20-rc5/fs/ufs/balloc.c
===================================================================
--- linux-2.6.20-rc5.orig/fs/ufs/balloc.c
+++ 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,

-- 
/Evgeniy