xfs
[Top] [All Lists]

Re: [PATCH 3/4] xfs: combine xfs_rtmodify_summary and xfs_rtget_summary

To: Brian Foster <bfoster@xxxxxxxxxx>
Subject: Re: [PATCH 3/4] xfs: combine xfs_rtmodify_summary and xfs_rtget_summary
From: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Fri, 22 Aug 2014 10:23:43 -0500
Cc: Eric Sandeen <sandeen@xxxxxxxxxx>, xfs-oss <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <53F75B54.8050802@xxxxxxxxxxx>
References: <53F6942B.80808@xxxxxxxxxx> <53F6963D.9090500@xxxxxxxxxxx> <20140822131950.GC3915@xxxxxxxxxxxxxx> <53F75B54.8050802@xxxxxxxxxxx>
On 8/22/14, 10:01 AM, Eric Sandeen wrote:
> On 8/22/14, 8:19 AM, Brian Foster wrote:
>> On Thu, Aug 21, 2014 at 08:00:45PM -0500, Eric Sandeen wrote:
> 
> ...
> 
>>> @@ -480,15 +484,40 @@ xfs_rtmodify_summary(
>>>             }
>>>     }
>>>     /*
>>> -    * Point to the summary information, modify and log it.
>>> +    * Point to the summary information, modify/log it, and/or copy it out.
>>>      */
>>>     sp = XFS_SUMPTR(mp, bp, so);
>>> -   *sp += delta;
>>> -   xfs_trans_log_buf(tp, bp, (uint)((char *)sp - (char *)bp->b_addr),
>>> -           (uint)((char *)sp - (char *)bp->b_addr + sizeof(*sp) - 1));
>>> +   if (delta) {
>>> +           uint first = (uint)((char *)sp - (char *)bp->b_addr);
>>> +
>>> +           *sp += delta;
>>> +           xfs_trans_log_buf(tp, bp, first, first + sizeof(*sp) - 1);
>>> +   }
>>> +   if (sum) {
>>> +           /*
>>> +            * Drop the buffer if we're not asked to remember it.
>>> +            */
>>> +           if (!rbpp)
>>> +                   xfs_trans_brelse(tp, bp);
>>
>> This introduces some potentially weird circumstances (e.g., acquire,
>> log, release of a buffer), but I think it's resolved by the next patch.
>>
>> Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> 
> does it introduce it, or just highlight it?  I thought it was weird too,
> but I think it existed before; that's what prompted me to go looking at
> callers and drop the rbpp checks, FWIW.

Oh, right, if delta & sum,there's a problem if (!rpbb).  And I did introduce
that as a new issue.  But the code at that point never sees (!rbpp) so I think
it's safe.  I could respin patches 3 & 4, to do them in the opposite order,
if desired.

-Eric

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