[Cluster-devel] [PATCH GFS2] gfs2_stuffed_write_end modifying source buffer?

[Cluster-devel] [PATCH GFS2] gfs2_stuffed_write_end modifying source buffer?

[Bob Peterson]

Hi,

Maybe I'm wrong, but this looks like a bug to me: It looks like
GFS2's function gfs2_stuffed_write_end is zeroing out portions
of the source buffer.  So if I create a character array and
filled it with "X" then wrote only one byte to a very
small file, all the other X's in my buffer would get nuked.
Just a theory at this point but perhaps Steve Whitehouse can tell.

Regards,

Bob Peterson
Red Hat File Systems
--
 fs/gfs2/aops.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
index 7ebae9a..6a23ba2 100644
--- a/fs/gfs2/aops.c
+++ b/fs/gfs2/aops.c
@@ -801,7 +801,6 @@ static int gfs2_stuffed_write_end(struct inode *inode, struct buffer_head *dibh,
 	BUG_ON((pos + len) > (dibh->b_size - sizeof(struct gfs2_dinode)));
 	kaddr = kmap_atomic(page, KM_USER0);
 	memcpy(buf + pos, kaddr + pos, copied);
-	memset(kaddr + pos + copied, 0, len - copied);
 	flush_dcache_page(page);
 	kunmap_atomic(kaddr, KM_USER0);
 

[Cluster-devel] Re: [PATCH GFS2] gfs2_stuffed_write_end modifying source buffer?

[Steven Whitehouse]

Hi,

I'm not convinced there is a bug here...

On Wed, 2009-09-23 at 17:48 -0400, Bob Peterson wrote:
> Hi,
> 
> Maybe I'm wrong, but this looks like a bug to me: It looks like
> GFS2's function gfs2_stuffed_write_end is zeroing out portions
> of the source buffer.  So if I create a character array and
> filled it with "X" then wrote only one byte to a very
> small file, all the other X's in my buffer would get nuked.
> Just a theory at this point but perhaps Steve Whitehouse can tell.
> 
> Regards,
> 
> Bob Peterson
> Red Hat File Systems
> --
>  fs/gfs2/aops.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
> index 7ebae9a..6a23ba2 100644
> --- a/fs/gfs2/aops.c
> +++ b/fs/gfs2/aops.c
> @@ -801,7 +801,6 @@ static int gfs2_stuffed_write_end(struct inode *inode, struct buffer_head *dibh,
>  	BUG_ON((pos + len) > (dibh->b_size - sizeof(struct gfs2_dinode)));
>  	kaddr = kmap_atomic(page, KM_USER0);
>  	memcpy(buf + pos, kaddr + pos, copied);
> -	memset(kaddr + pos + copied, 0, len - copied);
pos is the current file offset (which since its stuffed is also a byte
offset into this (first) page of the file)
copied is the number of bytes that were copied
len is the number of bytes that we wanted to copy (so for a successful
copy, its equal to copied and thus the memset() is a no-op

>  	flush_dcache_page(page);
>  	kunmap_atomic(kaddr, KM_USER0);
>  
The purpose of that memset() is to deal with the situation where the
copy has failed part way though (source page inaccessible) and thus we
couldn't complete the operation. If the write was not an extending
write, we cannot truncate off the bit which wasn't copied, so it gets
zeroed out instead.

Steve.