On Tue, May 10, 2011 at 7:38 PM, Robert Haas
<robertmhaas@gmail.com> wrote:
On Tue, May 10, 2011 at 9:45 AM, Merlin Moncure <
mmoncure@gmail.com> wrote:
> I see: here's a comment that was throwing me off:
> + /*
> + * If we didn't get the lock and it turns out we need it, we'll have to
> + * unlock and re-lock, to avoid holding the buffer lock across an I/O.
> + * That's a bit unfortunate, but hopefully shouldn't happen often.
> + */
>
> I think that might be phrased as "didn't get the pin and it turns out
> we need it because the bit can change after inspection". The visible
> bit isn't 'wrong' as suggested in the comments, it just can change so
> that it becomes wrong. Maybe a note of why it could change would be
> helpful.
point is well-taken as well. Here's a revised version with some
additional wordsmithing.
Some trivial comments.
Why do the set and clear functions need pass-by-reference (Buffer *) argument ? I don't see them modifying the argument at all. This patch adds the clear function, but the existing set function also suffers from that.
There are several invocations of pin/clear/release combos. May be you would want a convenience routine for doing that in a single step or just pass InvalidBuffer to clear() in which case, it would assume that the vm buffer is not pinned and do the needful.
The comment at the top of visibilitymap_pin_ok says "On entry, *buf", but the function really takes just a buf. You can possibly fold visibilitymap_pin_ok() into a macro (and also name it slightly differently like visibilitymap_is_pinned ?).
Thanks,
Pavan