Treat writes as new when holes span across page boundaries

When a hole spans across page boundaries, the next write forces a read of the block. This could end up reading existing garbage data from the disk in ocfs2_map_page_blocks. This leads to non-zero holes. In order to avoid this, mark the writes as new when the holes span across page boundaries.

---

fs/ocfs2/aops.c +0 -0

index 1fbb0e2..4be220d 100644

Quoted 5 lines · Goldwyn Rodrigues

When a hole spans across page boundaries, the next write forces a read of the block. This could end up reading existing garbage data from the disk in ocfs2_map_page_blocks. This leads to non-zero holes. In order to avoid this, mark the writes as new when the holes span across page boundaries.

? ? ? ?Is this a new approach to your earlier patch, or an additional change?

Quoted 11 lines

---

fs/ocfs2/aops.c +0 -0

index 1fbb0e2..4be220d 100644
@@ -1026,6 +1026,12 @@ static int ocfs2_prepare_page_for_write(struct
 inode *inode, u64 *p_blkno,
 ? ? ? ocfs2_figure_cluster_boundaries(OCFS2_SB(inode->i_sb), cpos,
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &cluster_start, &cluster_end);
 

fs/ocfs2/aops.c static int ocfs2_prepare_page_for_write(struct · line 1034

Quoted 5 lines

fs/ocfs2/aops.c +5 -0

+ ? ? /* treat the write as new if the a hole/lseek spanned across
+ ? ? ?* the page boundary.
+ ? ? ?*/
+ ? ? new = new | ((i_size_read(inode) <= page_offset(page)) &&
+ ? ? ? ? ? ? ? ? ? ? (page_offset(page) <= user_pos));

? ? ? ?There are two problems here. ?First, It's not safe to claim existing data is 'new'. ?Imagine you have a 4K page and a 512B blocksize. ?The first 2 blocks of the page have data in them, but your code change will cause them to be set_uptodate() even if we haven't read them in yet.

? ? ? ?Secondly, ocfs2_should_read_blk() already checks for blocks past i_size and skips reading them. ?So if you are trying to avoid reading them, it is already handled.

Quoted 2 lines · Joel Becker

? ? ? ?Is this a new approach to your earlier patch, or an additional change?

This is a new approach. You can scrap the previous attempt. I started reading about write prepare's once you advised to look at the write front.

Quoted 10 lines

Quoted 10 lines

---

fs/ocfs2/aops.c +0 -0

index 1fbb0e2..4be220d 100644
@@ -1026,6 +1026,12 @@ static int ocfs2_prepare_page_for_write(struct
 inode *inode, u64 *p_blkno,
 ? ? ? ocfs2_figure_cluster_boundaries(OCFS2_SB(inode->i_sb), cpos,
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &cluster_start, &cluster_end);
 

Quoted 11 lines

fs/ocfs2/aops.c static int ocfs2_prepare_page_for_write(struct · line 1034

Quoted 5 lines

fs/ocfs2/aops.c +5 -0

+ ? ? /* treat the write as new if the a hole/lseek spanned across
+ ? ? ?* the page boundary.
+ ? ? ?*/
+ ? ? new = new | ((i_size_read(inode) <= page_offset(page)) &&
+ ? ? ? ? ? ? ? ? ? ? (page_offset(page) <= user_pos));

? ? ? ?There are two problems here. ?First, It's not safe to claim existing data is 'new'. ?Imagine you have a 4K page and a 512B blocksize. ?The first 2 blocks of the page have data in them, but your code change will cause them to be set_uptodate() even if we haven't read them in yet.

I did not understand this. Could you explain this in terms of blocksize=512, page_offset, i_size and pos?
However I'll try to interpret: Won't the i_size be 1024 (assuming it is the start of file) and the new flag set to false?

Quoted 3 lines

? ? ? ?Secondly, ocfs2_should_read_blk() already checks for blocks past i_size and skips reading them. ?So if you are trying to avoid reading them, it is already handled.

If the previous call was lseek, i_size is not updated. ocfs2_should_read_blk returns true and junk read.

Quoted 7 lines · Goldwyn Rodrigues

Quoted 2 lines · Joel Becker

? ? ? ?Is this a new approach to your earlier patch, or an additional change?

This is a new approach. You can scrap the previous attempt. I started reading about write prepare's once you advised to look at the write front.

? ? ? ?Ok, cool. ?It definitely is along the track I was suggesting.

Quoted 22 lines

Quoted 21 lines

Quoted 6 lines

---

fs/ocfs2/aops.c +0 -0

index 1fbb0e2..4be220d 100644

fs/ocfs2/aops.c static int ocfs2_prepare_page_for_write(struct · line 1034

Quoted 9 lines

fs/ocfs2/aops.c +5 -0

@@ -1026,6 +1026,12 @@ static int ocfs2_prepare_page_for_write(struct
 inode *inode, u64 *p_blkno,
 ? ? ? ocfs2_figure_cluster_boundaries(OCFS2_SB(inode->i_sb), cpos,
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &cluster_start, &cluster_end);
 
+ ? ? /* treat the write as new if the a hole/lseek spanned across
+ ? ? ?* the page boundary.
+ ? ? ?*/
+ ? ? new = new | ((i_size_read(inode) <= page_offset(page)) &&
+ ? ? ? ? ? ? ? ? ? ? (page_offset(page) <= user_pos));

? ? ? ?There are two problems here. ?First, It's not safe to claim existing data is 'new'. ?Imagine you have a 4K page and a 512B blocksize. ?The first 2 blocks of the page have data in them, but your code change will cause them to be set_uptodate() even if we haven't read them in yet.

Quoted 4 lines

I did not understand this. Could you explain this in terms of blocksize=512, page_offset, i_size and pos?
However I'll try to interpret: Won't the i_size be 1024 (assuming it is the start of file) and the new flag set to false?

? ? ? ?What if i_size is 1000? ?The second block will have data,

including zeros to the end of the block. ?I have to think about this some more to be sure where the offsets lie -- this is hard code to think about -- but essentially there is a difference between blocks we have allocated but not used yet, and blocks that are part of a brand new allocation. ?We need to make sure we're not confusing the two.

Quoted 6 lines

Quoted 3 lines

? ? ? ?Secondly, ocfs2_should_read_blk() already checks for blocks past i_size and skips reading them. ?So if you are trying to avoid reading them, it is already handled.

If the previous call was lseek, i_size is not updated. ocfs2_should_read_blk returns true and junk read.

? ? ? ?Why would it return true if the f_pos is > i_size? ?It should return 0.

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

? ? ? ?Secondly, ocfs2_should_read_blk() already checks for blocks past i_size and skips reading them. ?So if you are trying to avoid reading them, it is already handled.

Sorry, I was wrong in my previous explanation on why ocfs2_should_read_blk returns 1. ocfs2_should_read_blk returns 1 because of ocfs2_sparse_alloc() and hence forces the garbage read.

On a different note, what I have not been able to explain as yet is, when the file is created on a nosparse filesystem, the holes have junk (not 0xbaadfeed, ie what was written previously but junk). Could you think of a reason why? In any case, this patch resolves this issue.

Please try the attached patch and see if it fixes the problem.
It should work on its own, without your changes. If I have it wrong, we'll continue to evaluate the problem. I'd test it myself, but my VM setup is currently broken.

Quoted 8 lines · Goldwyn Rodrigues

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

? ? ? ?Secondly, ocfs2_should_read_blk() already checks for blocks past i_size and skips reading them. ?So if you are trying to avoid reading them, it is already handled.

Sorry, I was wrong in my previous explanation on why ocfs2_should_read_blk returns 1. ocfs2_should_read_blk returns 1 because of ocfs2_sparse_alloc() and hence forces the garbage read.

? ? ? ?You're right, that's why ocfs2_should_read_block() returns 1. But why would it ever want to read blocks that are outside i_size? We've guaranteed that they won't be initialized...
? ? ? ?I wonder if that's the bug? ?We originally used to zero to EOC. For sparse files, that could never be any clusters past i_size. ?With the fix I put in for writepage, we no longer zero the blocks past i_size. ?I think the sparse check there is no longer accurate. ? ? ? ?Please try the attached patch and see if it fixes the problem. It should work on its own, without your changes. ?If I have it wrong, we'll continue to evaluate the problem. ?I'd test it myself, but my VM setup is currently broken.

Quoted 4 lines

On a different note, what I have not been able to explain as yet is, when the file is created on a nosparse filesystem, the holes have junk (not 0xbaadfeed, ie what was written previously but junk). Could you think of a reason why? In any case, this patch resolves this issue.

? ? ? ?Do they have junk without your patch, or just with your patch?

Quoted 5 lines · Joel Becker

? ? ? ?Please try the attached patch and see if it fixes the problem. It should work on its own, without your changes. ?If I have it wrong, we'll continue to evaluate the problem. ?I'd test it myself, but my VM setup is currently broken.

It does not work. However, it shows the behavior similar to "nosparse" without patch. So, I would say what you are targeting is achieved but another problem resurfaces. This is because nothing zeros pages beyond i_size in ocfs2_map_page_blocks(), since we return early because of -

        if (ret == 0 || !new)
                return ret;

Quoted 12 lines · Goldwyn Rodrigues

Quoted 5 lines · Joel Becker

? ? ? ?Please try the attached patch and see if it fixes the problem. It should work on its own, without your changes. ?If I have it wrong, we'll continue to evaluate the problem. ?I'd test it myself, but my VM setup is currently broken.

It does not work. However, it shows the behavior similar to "nosparse" without patch. So, I would say what you are targeting is achieved but another problem resurfaces. This is because nothing zeros pages beyond i_size in ocfs2_map_page_blocks(), since we return early because of -

Quoted 2 lines

        if (ret == 0 || !new)
                return ret;

	Returning here is correct, because ret should == 0.  The new

part is about zeroing in the case of error. We should obviously handle it better before we get to this line.
I like that we come in line nosparse and sparse. What does
tailtest do for you? Does it fail or succeed?

Quoted 13 lines · Joel Becker

Quoted 12 lines · Goldwyn Rodrigues

Quoted 5 lines · Joel Becker

? ? ? ?Please try the attached patch and see if it fixes the problem. It should work on its own, without your changes. ?If I have it wrong, we'll continue to evaluate the problem. ?I'd test it myself, but my VM setup is currently broken.

It does not work. However, it shows the behavior similar to "nosparse" without patch. So, I would say what you are targeting is achieved but another problem resurfaces. This is because nothing zeros pages beyond i_size in ocfs2_map_page_blocks(), since we return early because of -

Quoted 8 lines

Quoted 2 lines

? ? ? ? if (ret == 0 || !new)
? ? ? ? ? ? ? ? return ret;

? ? ? Returning here is correct, because ret should == 0. ?The new part is about zeroing in the case of error. ?We should obviously handle it better before we get to this line.
? ? ? I like that we come in line nosparse and sparse. ?What does tailtest do for you? ?Does it fail or succeed?

? ? ? ?Looking some more. ?ocfs2_zero_tail() is supposed to handle this, isn't it? ?ocfs2_write_begin_nolock() calls ocfs2_zero_tail(pos), which calls ocfs2_zero_extend(pos), which works up to ocfs2_clusters_for_bytes(pos). ?Doesn't this include your cluster? Shouldn't the page already be loaded into the pagecache with zeros?

Quoted 8 lines · Joel Becker

On Tue, Feb 22, 2011 at 01:39:28PM -0800, Joel Becker wrote:
? ? ? ?Looking some more. ?ocfs2_zero_tail() is supposed to handle this, isn't it? ?ocfs2_write_begin_nolock() calls ocfs2_zero_tail(pos), which calls ocfs2_zero_extend(pos), which works up to ocfs2_clusters_for_bytes(pos). ?Doesn't this include your cluster? Shouldn't the page already be loaded into the pagecache with zeros?

Din't you discard that idea in my previous attempt? ;)

On a serious note, no it doesn't because zero_start and zero_to_size are the same when pos is on a block boundary and hence the loop does not execute.

Quoted 11 lines · Goldwyn Rodrigues

Quoted 8 lines · Joel Becker

On Tue, Feb 22, 2011 at 01:39:28PM -0800, Joel Becker wrote:
? ? ? ?Looking some more. ?ocfs2_zero_tail() is supposed to handle this, isn't it? ?ocfs2_write_begin_nolock() calls ocfs2_zero_tail(pos), which calls ocfs2_zero_extend(pos), which works up to ocfs2_clusters_for_bytes(pos). ?Doesn't this include your cluster? Shouldn't the page already be loaded into the pagecache with zeros?

Din't you discard that idea in my previous attempt? ;)

? ? ? ?I might have ;-) ?I'm reloading state here, so I might be coming back to something you've already seen. ?I'm trying to ask questions that make both of us come to the same understanding. ?So my apologies if I didn't get what you were saying before.
? ? ? ?For example, I thought you were saying that a write from 0 to 1023 (new i_size of 1024), with a blocksize of 4K and a clustersize of 8K, should zero from 1024 to 8191. ?That is, zeroing the entire cluster at allocation time, even though the second block does not contain i_size. ?My contention is that 1024 to 4095 should be zeroed for this write. ?We would only zero the second block when the file is later extended past 4096.
? ? ? ?I may have been confused because ocfs2_zero_extend() only used to matter for nonsparse, where this behavior was different. ?Which of the above did you mean?

Quoted 3 lines

On a serious note, no it doesn't because zero_start and zero_to_size are the same when pos is on a block boundary and hence the loop does not execute.

? ? ? ?If they are the same (i_size == pos), there is no hole. ?I don't see how there is a problem.

Quoted 4 lines · Joel Becker

? ? ? ?I may have been confused because ocfs2_zero_extend() only used to matter for nonsparse, where this behavior was different. ?Which of the above did you mean?

Not now, but it may in the future and that is the problem.

So consider your case of usual parameters, but i_size=4096. Now you append 32 bytes of data to it, so pos = i_size = 4096 and len=32
write(fd, 32 bytes of data);

In this case, zero_start and zero_to_size in zero_extend_file are both 4096 and it will not zero this page in ocfs2_zero_extend.

After the write, i_size=4128.

Now lseek 512 bytes -
lseek(fd, 512, SEEKCUR);

..and write 4096 bytes again.
write(fd, 4096bytes of data);

pos=4640 len=4096 bytes, and after the transaction i_size=8736.

i_size is beyond 8192 now

Hole between 4128-4640 was never zeroed and carries junk. Does that make sense or am I doing the math wrong here?

Quoted 5 lines · Goldwyn Rodrigues

Quoted 4 lines · Joel Becker

? ? ? ?I may have been confused because ocfs2_zero_extend() only used to matter for nonsparse, where this behavior was different. ?Which of the above did you mean?

Key:
?EOF -> End of File, or i_size.
?EOB -> End of Block, the end of the block containing i_size. ?So if ? ? ? ?i_size is 8000 and blocksize is 4096, EOB is 8192. ?EOC -> End of Cluster, the end of the cluster containing i_size. ?So if ? ? ? ?i_size is 8000, blocksize is 4096, and clustersize is 16K, EOC ? ? ? ?is 16384, two blocks past i_size.

Hey Goldwyn,
? ? ? ?I want to apologize for not quite getting the point of your original patch. ?The legacy ocfs2_zero_extend() name put me in the mind of our old nonsparse allocation code, and I saw EOF->EOC and assumed it was at allocation time, not i_size change time. ?I knew I was still reloading the state in my brain and could be missing something, but that still must have been frustrating to you.

Quoted 9 lines

Not now, but it may in the future and that is the problem.

So consider your case of usual parameters, but i_size=4096. Now you append 32 bytes of data to it, so pos = i_size = 4096 and len=32
write(fd, 32 bytes of data);

In this case, zero_start and zero_to_size in zero_extend_file are both 4096 and it will not zero this page in ocfs2_zero_extend.

? ? ? ?Correct.

Quoted 1 line

After the write, i_size=4128.

? ? ? ?And the block is zeroed out to 8191 (or at least it should be) by.

Quoted 7 lines

Now lseek 512 bytes -
lseek(fd, 512, SEEKCUR);

..and write 4096 bytes again.
write(fd, 4096bytes of data);

Quoted 6 lines

pos=4640 len=4096 bytes, and after the transaction i_size=8736.

i_size is beyond 8192 now

Hole between 4128-4640 was never zeroed and carries junk. Does that make sense or am I doing the math wrong here?

? ? ? ?That may be the problem, but that is not what should have happened. ?write_begin() should have zeroed 4128-8192 while processing the 32-byte write. ?Linux always assumes that you zero a block when you bring it into service. ?Now, this is not the responsibility of ocfs2_zero_tail(), because i_size == pos. ?ocfs2_zero_tail() is supposed to work when i_size < pos. ?It's when you prepare the pages that this matters.

? ? ? ?Think about your first patch. ?It zeros from i_size to pos. ?But

we're supposed to be guaranteed that zeros exist from i_size to EOB. Thus I read it as zeroing during allocation, not when i_size moves to cover already allocated clusters. ?The "correct" approach (as Linux sees it) would zero from EOB to pos (or, actually, EOB to the start of the block containing pos). ?We're already supposed to be doing that; that's part of what I recently fixed. ?Later in in our discussion I thought you might have found a bug in my fix. ?That's why I looked at the ocfs2_should_read_block() code. ?Now I see that is not the case (though my patch is probably correct for ocfs2_should_read_block(), it is unrelated to the problem you are seeing).

? ? ? ?Given your example above, at write(fd, 32 bytes of data), ocfs2_prepare_pages_for_write() should be zeroing 4128-8192 before the write code is allowed to copy the 32 bytes into 4096-4128. ?Everyone expects it to work this way (xfs, extN, etc). ?That's exactly what write_begin() is for. ?It gives back a page array where all the pages are properly allocated and zeroed. ?Ours is complex because we have to handle cluster allocation, where clusters, blocks, and pages can be all be various multiples of each other.

? ? ? ?Your last patch, where you force 'new', isn't quite right. ?The cluster boundaries passed to map_pages aren't right, and the later zeroing could be wrong (zeroing earlier parts of the file that contained data). ?At least, that's how I read it. ?But I think you're in the right place to start.

Quoted 5 lines · Joel Becker

Quoted 1 line

After the write, i_size=4128.

? ? ? ?And the block is zeroed out to 8191 (or at least it should be) by.

by...?

So, who should zero the page? I assume map_pages

Quoted 1 line

? ? ? ?Think about your first patch. ?It zeros from i_size to pos. ?But

Actually, it zeroed from i_size to EOC. But again I think that might be incorrect because of the commits you mentioned.

Quoted 5 lines

? ? ? ?Your last patch, where you force 'new', isn't quite right. ?The cluster boundaries passed to map_pages aren't right, and the later zeroing could be wrong (zeroing earlier parts of the file that contained data). ?At least, that's how I read it. ?But I think you're in the right place to start.

I still don't understand why? I am not considering clusters at all. The flag is set to new if -
i_size <= page_offset <= pos

The calling code should have made sure we are correctly passing should_zero to all non-target pages. Having new overridden for them is wrong. So maybe we move your check inside the target_page if. It only really matters for the target page.

But wait! Check out block_write_full_page_endio() in
fs/buffer.c! Especially this part:

	zero_user_segment(page, offset, PAGE_CACHE_SIZE);

It is zeroing the tail of a page when it straddles i_size to make sure it goes out correctly. But, of course, blocks past i_size never go to disk. And we don't care; readpage should handle them correctly, right? So if writepage is going to zero for us, and readpage is going to read correctly, how are you getting bad data?
I was about to think we were done, but now I'm not so sure. I
have a question: with my change to ocfs2_should_read_block(), but without your patch, what do you see for corruption? Your pattern written in the first pass, or complete random garbage?

Quoted 4 lines · Joel Becker

The calling code should have made sure we are correctly passing should_zero to all non-target pages. ?Having new overridden for them is wrong. ?So maybe we move your check inside the target_page if. ?It only really matters for the target page.

? ? ? ?Actually, I have a question: what is the clustersize of your test? ?Because if the filesystem is 4k/4k, the write at 4096 of 32 bytes should allocate a new cluster. ?'new' should already be set, and your check irrelevant.

Quoted 6 lines

? ? ? But wait! ?Check out block_write_full_page_endio() in fs/buffer.c! ?Especially this part:

? ? ? zero_user_segment(page, offset, PAGE_CACHE_SIZE);

Quoted 9 lines

It is zeroing the tail of a page when it straddles i_size to make sure it goes out correctly. ?But, of course, blocks past i_size never go to disk. ?And we don't care; readpage should handle them correctly, right? So if writepage is going to zero for us, and readpage is going to read correctly, how are you getting bad data?
? ? ? I was about to think we were done, but now I'm not so sure. ?I have a question: with my change to ocfs2_should_read_block(), but without your patch, what do you see for corruption? ?Your pattern written in the first pass, or complete random garbage?

? ? ? ?One thing I would like to see are the results with fsync() between writeSparse() and checkSparselyWrittenFile(). ?Forcing the file to disk before checking the values will tell us something. ?This is with my patch but without yours.
? ? ? ?Another good test would be to umount/mount between writeSparse() and checkSparselyWrittenFile(). ?This tells us what actually hit disk.

Quoted 6 lines · Joel Becker

Quoted 4 lines · Joel Becker

The calling code should have made sure we are correctly passing should_zero to all non-target pages. ?Having new overridden for them is wrong. ?So maybe we move your check inside the target_page if. ?It only really matters for the target page.

Quoted 4 lines

? ? ? ?Actually, I have a question: what is the clustersize of your test? ?Because if the filesystem is 4k/4k, the write at 4096 of 32 bytes should allocate a new cluster. ?'new' should already be set, and your check irrelevant.

It is 16K and does not happen for clustersize==pagesize/blocksize. Don't have a machine with a bigger pagesize handy.

Quoted 9 lines

Quoted 5 lines

? ? ? ?Your last patch, where you force 'new', isn't quite right. ?The cluster boundaries passed to map_pages aren't right, and the later zeroing could be wrong (zeroing earlier parts of the file that contained data). ?At least, that's how I read it. ?But I think you're in the right place to start.

I still don't understand why? I am not considering clusters at all. The flag is set to new if -
i_size <= page_offset <= pos

? ? ? ?Thinking about it, I think you're right about this part of its safety. ?My big concern is that 'new' means "I have a whole empty cluster" and that might not be true.
? ? ? ?It's starting to look like your code is even closer to correct, but might not be located exactly where it needs to be. ?Let's look at what should_zero means.
? ? ? ?In ocfs2_write_cluster(), it means the calling functions have an entire new or unwritten cluster. ?That's why we take v_blkno as cpos when should_zero is true. ?We know that the caller has set us up with an entire cluster and we have to zero the front or all of it. ?That's great when we call down to ocfs2_prepare_page_for_write(), as we are able to trust we can zero anything we see with impunity. ?That's why we zero whole ranges (cluster_start..cluster_end). ?Remember here that a write context can have multiple pages (to cover the front of a large cluster that's just been allocated) or multiple clusters per page (16K/64K page size). ?should_zero (from c_needs_zero) tells us that the entire cluster we're looking at is newly active.
? ? ? ?Down in ocfs2_map_page_blocks(), we must not venture outside from/to, as other code (not the write path) assumes it will not pull in blocks that it doesn't need. ?So that's not a place to change. ? ? ? ?What about the loop at the bottom of ocfs2_write_cluster()? ?The one that calls ocfs2_prepare_page_for_write()? ?It can walk multiple pages, some of which may be the front of a newly allocated cluster (Imagine this write is allocating a 1MB cluster at the end of the file, but pos is 512K inside the new cluster. ?This code handles zeroing all the pages between the start of the cluster and pos). ?I suppose we could check here, but ocfs2_prepare_page_for_write() has logic based on target vs non-target page, and we don't want to override it.
? ? ? ?So we do want to make a change inside ocfs2_prepare_page_for_write(). ?Let's look at your change. ?You trigger 'new' even if we're not the target page. ?This means the BUG_ON() in the else{} can never fail for the extend I described in the last paragraph. The calling code should have made sure we are correctly passing should_zero to all non-target pages. ?Having new overridden for them is wrong. ?So maybe we move your check inside the target_page if. ?It only really matters for the target page.
? ? ? ?But wait! ?Check out block_write_full_page_endio() in fs/buffer.c! ?Especially this part:

? ? ? ?zero_user_segment(page, offset, PAGE_CACHE_SIZE);

It is zeroing the tail of a page when it straddles i_size to make sure it goes out correctly. ?But, of course, blocks past i_size never go to disk. ?And we don't care; readpage should handle them correctly, right? So if writepage is going to zero for us, and readpage is going to read correctly, how are you getting bad data?
? ? ? ?I was about to think we were done, but now I'm not so sure. ?I have a question: with my change to ocfs2_should_read_block(), but without your patch, what do you see for corruption? ?Your pattern written in the first pass, or complete random garbage?

Quoted 4 lines · Joel Becker

? ? ? ?I was about to think we were done, but now I'm not so sure. ?I have a question: with my change to ocfs2_should_read_block(), but without your patch, what do you see for corruption? ?Your pattern written in the first pass, or complete random garbage?

Joel, you are getting confused in the list of parameters to reproduce the bug. I have listed it in the past, but not in a single location so it is difficult to pick up the bits. I will put it together for you.

1. blocksize < clustersize

2. the previous write ended in a page boundary, but not a cluster boundary. eg, clustersize=16K but pos is at the page boundary = 4k

3. you wrote something at the page boundary

4. there is a hole created in the page from 4k-8k in *between* say 4128-4640

What I am seeing is pure junk(not 0xbaadfeed), and not what I had written earlier.

In retrospect, since we have covered the rest of the bases, we can set new on the condition:

i_size == page_offset == pos

I interpreted new as new page or new data vs new cluster, and perhaps that is the reason I placed the condition there.

Quoted 9 lines · Goldwyn Rodrigues

Quoted 4 lines · Joel Becker

? ? ? ?I was about to think we were done, but now I'm not so sure. ?I have a question: with my change to ocfs2_should_read_block(), but without your patch, what do you see for corruption? ?Your pattern written in the first pass, or complete random garbage?

Joel, you are getting confused in the list of parameters to reproduce the bug. I have listed it in the past, but not in a single location so it is difficult to pick up the bits. I will put it together for you.

Goldwyn,
? ? ? ?I'm just not remembering off the top of my head, so I'm asking. I'm actually describing the myriad cases we can encounter in this code, because any change we make has to be safe for all possible cases.

Quoted 1 line

1. blocksize < clustersize

? ? ? ?Ok.

Quoted 2 lines

2. ?the previous write ended in a page boundary, but not a cluster boundary. eg, clustersize=16K but pos is at the page boundary = 4k

? ? ? ?Must it be a page boundary? ?What about a block boundary? ;-)

Quoted 2 lines

3. you wrote something at the page boundary

? ? ? ?I think it's more block than page, really. ?But that is the same for 4K blocks and pages.

Quoted 1 line

4. there is a hole created in the page from 4k-8k in *between* say 4128-4640

? ? ? ?There is no hole here. ?We have allocation. ?Even if the user doesn't directly write to this location, it is not a hole, because we have a cluster there. ?You just mean that the user didn't write to this location (sparse writing). ?That's just a terminology thing, but it's important for other folks understanding you.

Quoted 3 lines

What I am seeing is pure junk(not 0xbaadfeed), and not what I had written earlier.

? ? ? ?Ok. ?That's what I expected. ?I wanted to clarify we were not reading stale data, but were instead failing to zero memory.

Quoted 3 lines

In retrospect, since we have covered the rest of the bases, we can set new on the condition:

Quoted 4 lines

i_size == page_offset == pos

I interpreted new as new page or new data vs new cluster, and perhaps that is the reason I placed the condition there.

? ? ? ?I don't like this. ?We still haven't figured out where we're failing.

? ? ? ?The core piece of this, why I'm going back and forth with you discussing the issue, is that we never want to paper over a symptom. Sure, we know that i_size==page_offset==pos triggers this, but is it the only case? ?If we figure out *why* we're here, when the code is supposed to not have this problem, then we can be sure we've closed out all exposure.

? ? ? ?To wit: If we really only have a problem when i_size is on a page boundary, doesn't that sound like an off-by-one error? ?We've had those before. ?I'm not sure that it is, but shouldn't we be sure about it?

Quoted 4 lines · Joel Becker

Quoted 2 lines

2. ?the previous write ended in a page boundary, but not a cluster boundary. eg, clustersize=16K but pos is at the page boundary = 4k

? ? ? ?Must it be a page boundary? ?What about a block boundary? ;-)

I am not sure of this, but I would like to stick with page boundary. I am too poor to afford a machine with a page size bigger than 4k to test this ;)

Quoted 4 lines · Goldwyn Rodrigues

Quoted 3 lines · Joel Becker

Quoted 2 lines

2. ?the previous write ended in a page boundary, but not a cluster boundary. eg, clustersize=16K but pos is at the page boundary = 4k

Quoted 5 lines

Quoted 1 line

? ? ? ?Must it be a page boundary? ?What about a block boundary? ;-)

I am not sure of this, but I would like to stick with page boundary. I am too poor to afford a machine with a page size bigger than 4k to test this ;)

Create a filesystem with 1K blocks and 16K clusters. Now you
have four blocks per page, and you can test block vs page boundaries.

Quoted 5 lines · Joel Becker

Quoted 4 lines · Goldwyn Rodrigues

Quoted 3 lines · Joel Becker

Quoted 2 lines

2. ?the previous write ended in a page boundary, but not a cluster boundary. eg, clustersize=16K but pos is at the page boundary = 4k

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

? ? ? ?Must it be a page boundary? ?What about a block boundary? ;-)

I am not sure of this, but I would like to stick with page boundary. I am too poor to afford a machine with a page size bigger than 4k to test this ;)

? ? ? Create a filesystem with 1K blocks and 16K clusters. ?Now you have four blocks per page, and you can test block vs page boundaries.

? ? ? ?I would love to see you modify tailtest to expose this bug. ?You should be able to do it with one set of writes (your write(4k), write(32b), write(4K at someplace past 4K+32b)) test). ?It will be a lot easier to debug if it is a simple script rather than a bunch of C writes.

Here is a simple script. I will incorporate later into tailtest later.

FILENAME=/mnt/f2

for i in seq 0 256; do

	let s=$i*4096
	echo "a" | dd of=$FILENAME count=1 bs=1 seek=$s conv=notrunc 2>/dev/null
	let t=$s+4095
	echo "b" | dd of=$FILENAME count=1 bs=1 seek=$t conv=notrunc 2>/dev/null

done

Quoted 10 lines · Goldwyn Rodrigues

Here is a simple script. I will incorporate later into tailtest later.

FILENAME=/mnt/f2

for i in seq 0 256; do
? ? ? let s=$i*4096
? ? ? echo "a" | dd of=$FILENAME count=1 bs=1 seek=$s conv=notrunc 2>/dev/null ? ? ? let t=$s+4095
? ? ? echo "b" | dd of=$FILENAME count=1 bs=1 seek=$t conv=notrunc 2>/dev/null done

? ? ? ?You shouldn't need to do 256 runs. ?I would like to see directed tests that just hit one spot in a file and expose the corruption. ?For example, your described problem case should work like so:

?# Write the first three blocks of the file, getting us past inline_data ?dd if=/dev/urandom of=$FILENAME count=3 bs=4096
?# Write a byte in the next page
?dd if=/dev/urandom of=$FILENAME count=1 bs=1 seek=12228 conv=notrunc ?# Write after some partial-block portion, trying to expose failed zeroing ?dd if=/dev/urandom of=$FILENAME count=4084 bs=1 seek=12300 conv=notrunc

I would expect this to expose the problem as you've described for clustersize >= 8K.

Quoted 23 lines · Joel Becker

Quoted 10 lines · Goldwyn Rodrigues

Here is a simple script. I will incorporate later into tailtest later.

FILENAME=/mnt/f2

for i in seq 0 256; do
? ? ? let s=$i*4096
? ? ? echo "a" | dd of=$FILENAME count=1 bs=1 seek=$s conv=notrunc 2>/dev/null ? ? ? let t=$s+4095
? ? ? echo "b" | dd of=$FILENAME count=1 bs=1 seek=$t conv=notrunc 2>/dev/null done

? ? ? ?You shouldn't need to do 256 runs. ?I would like to see directed tests that just hit one spot in a file and expose the corruption. ?For example, your described problem case should work like so:

?# Write the first three blocks of the file, getting us past inline_data ?dd if=/dev/urandom of=$FILENAME count=3 bs=4096
?# Write a byte in the next page
?dd if=/dev/urandom of=$FILENAME count=1 bs=1 seek=12228 conv=notrunc ?# Write after some partial-block portion, trying to expose failed zeroing ?dd if=/dev/urandom of=$FILENAME count=4084 bs=1 seek=12300 conv=notrunc

Quoted 2 lines

I would expect this to expose the problem as you've described for clustersize >= 8K.

Yes, that will work as well, as long as you can differentiate between the random characters from urandom and the (unexpected) non-zero characters in the (userspace) holes. You can read the data from [12229-12299] in the file you have created and check for zeros.

If you don't want too many changes - s/256/3/ in the script I posted and check for the 4th block.

Quoted 6 lines · Goldwyn Rodrigues

Quoted 3 lines · Joel Becker

Quoted 2 lines · Goldwyn Rodrigues

Here is a simple script. I will incorporate later into tailtest later.

Quoted 4 lines

Yes, that will work as well, as long as you can differentiate between the random characters from urandom and the (unexpected) non-zero characters in the (userspace) holes. You can read the data from [12229-12299] in the file you have created and check for zeros.

? ? ? ?You're right about the urandom being bad for this. ?Maybe it should be zeros. ?But still.

? ? ? ?I would love to see you modify tailtest to expose this bug. ?You should be able to do it with one set of writes (your write(4k), write(32b), write(4K at someplace past 4K+32b)) test). ?It will be a lot easier to debug if it is a simple script rather than a bunch of C writes.

Hi Joel,

Quoted 7 lines · Joel Becker

? ? ? ?I would love to see you modify tailtest to expose this bug. ?You should be able to do it with one set of writes (your write(4k), write(32b), write(4K at someplace past 4K+32b)) test). ?It will be a lot easier to debug if it is a simple script rather than a bunch of C writes.

I have posted the patch for ocfs2-test on the test mailing list.

While coding the test, I discovered that the problem does not occur for sparse,inline-data. I thought it might be because it is the first test in the series so I changed the order and confirmed that the test does not fail for sparse,inline-data. Any ideas?

Quoted 8 lines · Goldwyn Rodrigues

Quoted 5 lines · Joel Becker

Quoted 1 line

After the write, i_size=4128.

? ? ? ?And the block is zeroed out to 8191 (or at least it should be) by.

by...?

? ? ? ?Haha, where'd the rest of that sentence go? ?By the page prepare code (prepare_write() in older linux, write_begin() in newer linux).

Quoted 1 line

So, who should zero the page? I assume map_pages

? ? ? ?ocfs2_map_page_blocks() explicitly describes itself as not handling the zeroing. ?Let's trust that for now.

Quoted 4 lines

Quoted 1 line

? ? ? ?Think about your first patch. ?It zeros from i_size to pos. ?But

Actually, it zeroed from i_size to EOC. But again I think that might be incorrect because of the commits you mentioned.

? ? ? ?Sure. ?But it shouldn't be necessary.

Quoted 9 lines

Quoted 5 lines

? ? ? ?Your last patch, where you force 'new', isn't quite right. ?The cluster boundaries passed to map_pages aren't right, and the later zeroing could be wrong (zeroing earlier parts of the file that contained data). ?At least, that's how I read it. ?But I think you're in the right place to start.

I still don't understand why? I am not considering clusters at all. The flag is set to new if -
i_size <= page_offset <= pos

? ? ? ?Thinking about it, I think you're right about this part of its safety. ?My big concern is that 'new' means "I have a whole empty cluster" and that might not be true.
? ? ? ?It's starting to look like your code is even closer to correct, but might not be located exactly where it needs to be. ?Let's look at what should_zero means.
? ? ? ?In ocfs2_write_cluster(), it means the calling functions have an entire new or unwritten cluster. ?That's why we take v_blkno as cpos when should_zero is true. ?We know that the caller has set us up with an entire cluster and we have to zero the front or all of it. ?That's great when we call down to ocfs2_prepare_page_for_write(), as we are able to trust we can zero anything we see with impunity. ?That's why we zero whole ranges (cluster_start..cluster_end). ?Remember here that a write context can have multiple pages (to cover the front of a large cluster that's just been allocated) or multiple clusters per page (16K/64K page size). ?should_zero (from c_needs_zero) tells us that the entire cluster we're looking at is newly active.
? ? ? ?Down in ocfs2_map_page_blocks(), we must not venture outside from/to, as other code (not the write path) assumes it will not pull in blocks that it doesn't need. ?So that's not a place to change. ? ? ? ?What about the loop at the bottom of ocfs2_write_cluster()? ?The one that calls ocfs2_prepare_page_for_write()? ?It can walk multiple pages, some of which may be the front of a newly allocated cluster (Imagine this write is allocating a 1MB cluster at the end of the file, but pos is 512K inside the new cluster. ?This code handles zeroing all the pages between the start of the cluster and pos). ?I suppose we could check here, but ocfs2_prepare_page_for_write() has logic based on target vs non-target page, and we don't want to override it.
? ? ? ?So we do want to make a change inside ocfs2_prepare_page_for_write(). ?Let's look at your change. ?You trigger 'new' even if we're not the target page. ?This means the BUG_ON() in the else{} can never fail for the extend I described in the last paragraph. The calling code should have made sure we are correctly passing should_zero to all non-target pages. ?Having new overridden for them is wrong. ?So maybe we move your check inside the target_page if. ?It only really matters for the target page.
? ? ? ?But wait! ?Check out block_write_full_page_endio() in fs/buffer.c! ?Especially this part:

? ? ? ?zero_user_segment(page, offset, PAGE_CACHE_SIZE);

Are you sure that block_write_full_page_endio() will be called before the next system call on this file comes in and moves I_size beyond end of this page?

When a hole spans across page boundaries, the next write forces a read of the block. This could end up reading existing garbage data from the disk in ocfs2_map_page_blocks. This leads to non-zero holes. In order to avoid this, mark the writes as new when the holes span across page boundaries.

Signed-off-by Goldwyn Rodrigues <[email protected]>