xfs
[Top] [All Lists]

Re: [PATCH v2] xfs: fix assertion failure in xfs_vm_write_failed()

To: Jeff Liu <jeff.liu@xxxxxxxxxx>
Subject: Re: [PATCH v2] xfs: fix assertion failure in xfs_vm_write_failed()
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 19 Mar 2013 10:30:26 +1100
Cc: "xfs@xxxxxxxxxxx" <xfs@xxxxxxxxxxx>, "Michael L. Semon" <mlsemon35@xxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <51469C90.1000001@xxxxxxxxxx>
References: <51469C90.1000001@xxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Mon, Mar 18, 2013 at 12:48:16PM +0800, Jeff Liu wrote:
> Hello,
> 
> Here is the v2 patch for fixing ASSERTION failed at xfs_vm_write_failed() 
> according
> to Dave's comments, so I added Dave as SOB for credit.

No, please don't add my SOB to a patch you wrote, even if I came up
with the idea of how to do something. A SOB indicates that someone
has verified the origin of the patch (e.g. for copyright reasons),
not who contributed to finding the problem.  IOWs, adding someone
else's SOB to a patch you wrote is almost always the wrong thing to
do.

The correct way to acknowledge someone's contribution to the
fix if they didn't write the patch is by a line in the commit
message saying something like "Thanks to .... for helping find and
fix the problem." [Edit: I just noticed you did this bit. ;) ]

If .... agrees with your fix, then they can add a reviewed-by line
to the patch, which has significantly different meaning to a SOB...

....

> Reported-by: Michael L. Semon <mlsemon35@xxxxxxxxx>
> Signed-off-by: Jie Liu <jeff.liu@xxxxxxxxxx>
> Signed-off-by: Dave Chinner <david@xxxxxxxxxxxxx>
> ---
>  fs/xfs/xfs_aops.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index 5f707e5..a418e17 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -1494,7 +1494,8 @@ xfs_vm_write_failed(
>       loff_t                  pos,
>       unsigned                len)
>  {
> -     loff_t                  block_offset = pos & PAGE_MASK;
> +     loff_t                  block_offset = (pos >> PAGE_CACHE_SHIFT) <<
> +                                             PAGE_CACHE_SHIFT;
>       loff_t                  block_start;
>       loff_t                  block_end;
>       loff_t                  from = pos & (PAGE_CACHE_SIZE - 1);

This needs a comment explaining why we aren't just masking the value
off with PAGE_MASK. And given that it wraps, something like:

-       loff_t                  block_offset = pos & PAGE_MASK;
+       loff_t                  block_offset;
.....

+       /*
+        * comment about 32 bit systems, 64 bit variables and masks
+        */
+       block_offset = (pos >> PAGE_CACHE_SHIFT) << PAGE_CACHE_SHIFT;

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>