[Top] [All Lists]

RE: [PATCH] xfs: check all buffers in xfs_check_page_type()

To: Shaun Gosse <sgosse@xxxxxxx>, Mark Tinguely <tinguely@xxxxxxx>, "Brian Foster" <bfoster@xxxxxxxxxx>
Subject: RE: [PATCH] xfs: check all buffers in xfs_check_page_type()
From: Shaun Gosse <sgosse@xxxxxxx>
Date: Fri, 28 Feb 2014 21:34:03 +0000
Accept-language: en-US
Cc: "xfs@xxxxxxxxxxx" <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <8D3FA7645C1CFC4E9E783D22B4C708647305CBB3@xxxxxxxxxxxxxxxxxxxxxxxxx>
References: <1393615369-41882-1-git-send-email-bfoster@xxxxxxxxxx> <5310EB67.5050404@xxxxxxx> <8D3FA7645C1CFC4E9E783D22B4C708647305CBB3@xxxxxxxxxxxxxxxxxxxxxxxxx>
Thread-index: AQHPNLqBxXjz/8WGzkqOXZ5SSc5f65rLezmA//+iKUCAABKKoA==
Thread-topic: [PATCH] xfs: check all buffers in xfs_check_page_type()
Ha, disregard, caught up to read the other email and I see everyone else got 
there too. I would've been in the thread an hour ago, but unfortunately timed 
call back from Microsoft support, much to my dismay.

Carry on then...

-----Original Message-----
From: xfs-bounces@xxxxxxxxxxx [mailto:xfs-bounces@xxxxxxxxxxx] On Behalf Of 
Shaun Gosse
Sent: Friday, February 28, 2014 3:33 PM
To: Mark Tinguely; Brian Foster
Cc: xfs@xxxxxxxxxxx
Subject: RE: [PATCH] xfs: check all buffers in xfs_check_page_type()

> Is there any reason to scan all the buffers when we all we want is an 
> indication that at least one is acceptable?
> Maybe there are generally not may buffers to a page to make it worthwhile.

The problem is the else doesn't check if acceptable is set before breaking. So 
it can quit early, not having found what it was looking for even though it was 
in one of the later buffers. Pre-mature optimization...

So the patch is good as is. It improves correctness.

If the optimization were desired, though, it could be:

+ else if (acceptable)
+   break 

In addition to the remove already posted, and with proper whitespace.

Fair warning: this is without looking at the code, just inferring from what's 
been written in those two emails. I'm hearing my CS101 professor in my head and 
thinking may not be worth a couple cycles at the risk of correctness or clarity 
(and suggesting code blind seems risky).

Actually, this could be the first condition. If the whole point of that loop is 
just to make sure to set acceptable if it can be set according to the rules on 
the basis of at least one of the buffers, then a starting:

+ if (acceptable)
+    break

With an annotation something like "/** Optimization: Quit when acceptable has 
been set. Loop has no other side-effects to worry about.*/" seems reasonable 
(and hopefully is true). And then it's clear from the code that's the point of 
the loop and how it works.


xfs mailing list

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