truncate fixes

fs/buffer.c +11 -0

@@ -1639,6 +1666,17 @@ static int __block_prepare_write(struct 
 	}
 	return 0;
 out:
+	bh = head;
+	block_start = 0;
+	do {
+		if (buffer_new(bh) && !buffer_uptodate(bh)) {
+			memset(kaddr+block_start, 0, bh->b_size);
+			set_bit(BH_Uptodate, &bh->b_state);
+			mark_buffer_dirty(bh);
+		}
+		block_start += bh->b_size;
+		bh = bh->b_this_page;
+	} while (bh != head);
 	return err;
 }
 

mm/filemap.c +7 -1

@@ -3004,7 +3004,7 @@ generic_file_write(struct file *file,con
 		kaddr = kmap(page);
 		status = mapping->a_ops->prepare_write(file, page, offset, offset+bytes);
 		if (status)
-			goto unlock;
+			goto sync_failure;
 		page_fault = __copy_from_user(kaddr+offset, buf, bytes);
 		flush_dcache_page(page);
 		status = mapping->a_ops->commit_write(file, page, offset, offset+bytes);

@@ -3029,6 +3029,7 @@ unlock:
 		if (status < 0)
 			break;
 	} while (count);
+done:
 	*ppos = pos;
 
 	if (cached_page)

@@ -3050,6 +3051,18 @@ out:
 fail_write:
 	status = -EFAULT;
 	goto unlock;
+
+sync_failure:
+	/*
+	 * If blocksize < pagesize, prepare_write() may have instantiated a
+	 * few blocks outside i_size.  Trim these off again.

I prefer my fix that simply recalls the ->truncate callback if -ENOSPC is returned by prepare_write. vmtruncate seems way overkill,

and after
calling ->truncate the __block_prepare_changes above won't be necessary because the leftover will be correctly deallocated (no need to clear them out and to mark them dirty, they will just go away before any readpage can see them).

Quoted 3 lines · Andrea Arcangeli

I prefer my fix that simply recalls the ->truncate callback if -ENOSPC is returned by prepare_write. vmtruncate seems way overkill,

No opinion on that here. This is what was in -ac. Perhaps Al can comment?

Quoted 3 lines · Andrea Arcangeli

I prefer my fix that simply recalls the ->truncate callback if -ENOSPC is returned by prepare_write. vmtruncate seems way overkill,

No opinion on that here. This is what was in -ac. Perhaps Al can comment?

Quoted 5 lines

and after
calling ->truncate the __block_prepare_changes above won't be necessary because the leftover will be correctly deallocated (no need to clear them out and to mark them dirty, they will just go away before any readpage can see them).

No, this code is needed if the write is _inside_ i_size, to an uninstantiated block. truncate won't remove those blocks, and we've gone and added them to the file.

-

I prefer my fix that simply recalls the ->truncate callback if -ENOSPC is returned by prepare_write. vmtruncate seems way overkill,

fs/buffer.c +3 -0

 	}
 	return 0;
 out:
+	bh = head;
+	block_start = 0;
+	do {

Quoted 6 lines · Andrew Morton

      }
      return 0;

out:

fs/buffer.c +3 -0

+     bh = head;
+     block_start = 0;
+     do {

fs/buffer.c static int __block_prepare_write(struct · line 1681

Quoted 10 lines

fs/buffer.c +8 -0

+             if (buffer_new(bh) && !buffer_uptodate(bh)) {
+                     memset(kaddr+block_start, 0, bh->b_size);
+                     set_bit(BH_Uptodate, &bh->b_state);
+                     mark_buffer_dirty(bh);
+             }
+             block_start += bh->b_size;
+             bh = bh->b_this_page;
+     } while (bh != head);
      return err;
 }

the above code will end marking uptodate (zeroed) buffers relative to blocks that cannot be read from disk. So a read-retry won't hit the disk and that's wrong.

I think that will be fixed by additionally also return -EIO in the wait_on_buffer loop (instead of goto out), so we won't generate zeroed uptodate cache in case of read failure.

Quoted 3 lines · Andrea Arcangeli

I prefer my fix that simply recalls the ->truncate callback if -ENOSPC is returned by prepare_write. vmtruncate seems way overkill,

Actually, vmtruncate will trim the page off the inode as well as the blocks from the file. I don't think there's necessarily a problem with having a page wholly outside i_size, but..

Quoted 19 lines · Andrea Arcangeli

Quoted 6 lines · Andrew Morton

      }
      return 0;

out:

fs/buffer.c +3 -0

+     bh = head;
+     block_start = 0;
+     do {

fs/buffer.c static int __block_prepare_write(struct · line 1681

Quoted 10 lines

fs/buffer.c +8 -0

+             if (buffer_new(bh) && !buffer_uptodate(bh)) {
+                     memset(kaddr+block_start, 0, bh->b_size);
+                     set_bit(BH_Uptodate, &bh->b_state);
+                     mark_buffer_dirty(bh);
+             }
+             block_start += bh->b_size;
+             bh = bh->b_this_page;
+     } while (bh != head);
      return err;
 }

Quoted 8 lines

the above code will end marking uptodate (zeroed) buffers relative to blocks that cannot be read from disk. So a read-retry won't hit the disk and that's wrong.

I think that will be fixed by additionally also return -EIO in the wait_on_buffer loop (instead of goto out), so we won't generate zeroed uptodate cache in case of read failure.

Right. There's also the case where get_block() returns -EIO when, for example, it fails on reading an indirect block. We end up writing zeroes into some of the blocks. But I think that behaviour is correct.

(I think I'll add a buffer_mapped() test to this code as well. It's a bit redundant because the fs shouldn't go setting BH_New and not BH_Mapped, but this code is _very_ rarely executed, and I haven't tested all filesystems...)

fs/buffer.c +2 -2

@@ -1633,12 +1660,22 @@ static int __block_prepare_write(struct 
         */
        while(wait_bh > wait) {
                wait_on_buffer(*--wait_bh);
-               err = -EIO;
                if (!buffer_uptodate(*wait_bh))
-                       goto out;
+                       return -EIO;
        }
        return 0;
 out:
+       bh = head;

fs/buffer.c +0 -0

        return err;
 }
 

I'll retest this, including the -EIO path.

Quoted 5 lines

(I think I'll add a buffer_mapped() test to this code as well. It's a bit redundant because the fs shouldn't go setting BH_New and not BH_Mapped, but this code is _very_ rarely executed, and I haven't tested all filesystems...)

correct, it shouldn't be necessary. I wouldn't add it. if a fs breaks the buffer_new semantics it's the one that should be fixed methinks.

Quoted 13 lines

fs/buffer.c +2 -2

@@ -1633,12 +1660,22 @@ static int __block_prepare_write(struct
         */
        while(wait_bh > wait) {
                wait_on_buffer(*--wait_bh);
-               err = -EIO;
                if (!buffer_uptodate(*wait_bh))
-                       goto out;
+                       return -EIO;
        }
        return 0;
 out:
+       bh = head;

fs/buffer.c static int __block_prepare_write(struct · line 1678

Quoted 10 lines

fs/buffer.c +10 -0

+       block_start = 0;
+       do {
+               if (buffer_new(bh) && buffer_mapped(bh) && !buffer_uptodate(bh)) {
+                       memset(kaddr+block_start, 0, bh->b_size);
+                       set_bit(BH_Uptodate, &bh->b_state);
+                       mark_buffer_dirty(bh);
+               }
+               block_start += bh->b_size;
+               bh = bh->b_this_page;
+       } while (bh != head);

I found another problem, we really need to keep track of which bh are been created by us during the failing prepare_write (buffer_new right now, not a long time ago), or we risk to corrupt data with a write passing over many bh, where the first bh of the page contained vaild data since a long time ago. To do this: 1) we either keep track of it on the kernel stack with some local variable or 2) we change the buffer_new semantics so that they indicate an "instant buffer_new" to clear just after checking it

Quoted 8 lines · Andrea Arcangeli

Quoted 5 lines

(I think I'll add a buffer_mapped() test to this code as well. It's a bit redundant because the fs shouldn't go setting BH_New and not BH_Mapped, but this code is _very_ rarely executed, and I haven't tested all filesystems...)

correct, it shouldn't be necessary. I wouldn't add it. if a fs breaks the buffer_new semantics it's the one that should be fixed methinks.

You mean "don't be lazy. Audit all the filesystems"? Sigh. OK.

Quoted 25 lines

Quoted 14 lines

fs/buffer.c +2 -2

@@ -1633,12 +1660,22 @@ static int __block_prepare_write(struct
         */
        while(wait_bh > wait) {
                wait_on_buffer(*--wait_bh);
-               err = -EIO;
                if (!buffer_uptodate(*wait_bh))
-                       goto out;
+                       return -EIO;
        }
        return 0;
 out:
+       bh = head;

fs/buffer.c static int __block_prepare_write(struct · line 1678

Quoted 10 lines

fs/buffer.c +10 -0

+       block_start = 0;
+       do {
+               if (buffer_new(bh) && buffer_mapped(bh) && !buffer_uptodate(bh)) {
+                       memset(kaddr+block_start, 0, bh->b_size);
+                       set_bit(BH_Uptodate, &bh->b_state);
+                       mark_buffer_dirty(bh);
+               }
+               block_start += bh->b_size;
+               bh = bh->b_this_page;
+       } while (bh != head);

Quoted 8 lines

I found another problem, we really need to keep track of which bh are been created by us during the failing prepare_write (buffer_new right now, not a long time ago), or we risk to corrupt data with a write passing over many bh, where the first bh of the page contained vaild data since a long time ago. To do this: 1) we either keep track of it on the kernel stack with some local variable or 2) we change the buffer_new semantics so that they indicate an "instant buffer_new" to clear just after checking it

Fair enough. How does this (untested) approach look?

(quoted diff) +12 -2

@@ -1600,6 +1627,7 @@ static int __block_prepare_write(struct 
                if (block_start >= to)
                        break;
                if (!buffer_mapped(bh)) {
+                       clear_bit(BH_New, &bh->b_state);
                        err = get_block(inode, block, bh, 1);
                        if (err)
                                goto out;

@@ -1633,12 +1661,30 @@ static int __block_prepare_write(struct 
         */
        while(wait_bh > wait) {
                wait_on_buffer(*--wait_bh);
-               err = -EIO;
                if (!buffer_uptodate(*wait_bh))
-                       goto out;
+                       return -EIO;
        }
        return 0;
 out:
+       /*
+        * Zero out any newly allocated blocks to avoid exposing stale
+        * data.  If BH_New is set, we know that the block was newly
+        * allocated in the above loop.
+        */
+       bh = head;
+       block_start = 0;
+       do {
+               if (buffer_new(bh)) {
+                       if (buffer_uptodate(bh))

Quoted 8 lines · Andrea Arcangeli

Quoted 5 lines

(I think I'll add a buffer_mapped() test to this code as well. It's a bit redundant because the fs shouldn't go setting BH_New and not BH_Mapped, but this code is _very_ rarely executed, and I haven't tested all filesystems...)

correct, it shouldn't be necessary. I wouldn't add it. if a fs breaks the buffer_new semantics it's the one that should be fixed methinks.

You mean "don't be lazy. Audit all the filesystems"? Sigh. OK.