On 9 August 2018 at 11:35, Steven Whitehouse <swhit...@redhat.com> wrote:
> Hi,
>
>
>
> On 08/08/18 19:52, Bob Peterson wrote:
>>
>> Hi,
>>
>> Before this patch, function foreach_descriptor repeatedly called
>> function gfs2_replay_incr_blk which just incremented the value while
>> decrementing another, and checked for wrap. This is a waste of time.
>> This patch just adds the value and adjusts it if a wrap occurred.
>>
>> Signed-off-by: Bob Peterson <rpete...@redhat.com>
>> ---
>>   fs/gfs2/recovery.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/gfs2/recovery.c b/fs/gfs2/recovery.c
>> index 0f501f938d1c..6c6b19263b82 100644
>> --- a/fs/gfs2/recovery.c
>> +++ b/fs/gfs2/recovery.c
>> @@ -354,8 +354,9 @@ static int foreach_descriptor(struct gfs2_jdesc *jd,
>> unsigned int start,
>>                         return error;
>>                 }
>>   -             while (length--)
>> -                       gfs2_replay_incr_blk(jd, &start);
>> +               start += length;
>> +               if (start >= jd->jd_blocks)
>> +                       start -= jd->jd_blocks;
>>                 brelse(bh);
>>         }
>>
>
> Now you've hidden the increment of the replay block. Please don't open code
> this, but just add an argument to gfs2_replay_incr_blk() such that you can
> tell it how many blocks to increment, rather than just assuming a single
> block as it does at the moment. Otherwise this can easily get missed when
> someone looks at the code in future, and expects gfs2_replay_incr_blk to be
> the only thing that changes the position during recovery,

If we really want to encapsulate "add modulo jd->jd_blocks", it's also
open-coded in find_good_lh and jhead_scan.

Andreas

Reply via email to