[Cluster-devel] [GFS2] Missing bit of readpages() fix

[Steven Whitehouse]

Hi,

On Thu, 2006-12-14 at 13:46 -0600, Russell Cattelan wrote:
> On Thu, 2006-12-14 at 18:43 +0000, Steven Whitehouse wrote:
> > Hi,
> > 
> > On Thu, 2006-12-14 at 12:34 -0600, Russell Cattelan wrote:
> > > On Thu, 2006-12-14 at 18:09 +0000, Steven Whitehouse wrote:
> > > > Hi,
> > > > 
> > > > The patch below is the missing bit of the readpages fix which hasn't yet
> > > > made it upstream. Its derived from Russell's original patch with a
> > > > slight change in that I've fixed up the original comment at the top of
> > > > the function rather than adding one in the body of the function. I'll
> > > > commit this tomorrow if there are no objections & also add it to the
> > > > pending RHEL5 patches,
> > > ya don't move the comment, it doesn't make much sense when it is moved
> > > out of context.
> > > 
> > Well we need to fix the comment at the top anyway, otherwise it would be
> > wrong and even more confusing, there didn't seem to be any reason to
> > mention it again since the patch removes most of the intervening lines.
> > 
> > > I'm still having trouble understanding why this patch is getting chopped
> > > up and rewritten? Doing so is significantly confusing things.
> > > 
> > What else is left to go upstream? As far as I know this is all there is
> > left for this particular fix as we have the rest of it already...
> Upstream looked strange: here is a version of the patch against
> upstream.
> (Comments fixed up)

I'm not sure what you mean by "looked strange". So far as I can tell
your version of the patch doesn't make any change to the logic, although
it does add some extra branches compared with my patch. In addition the
comment:

+               /*
+                * here due to a race with a truncate
+                * simply fail and let readpage handle the stuffed case
+                */
                ret = 0;

is wrong in so far as you don't get to that part of the code due to a
race in truncate, you get there because the file is stuffed. The race
with truncate is a pretty rare occurrence in "normal" use of the
filesystem, even though thats the branch thats taken when the race does
occur. I'm afraid that I really don't see what extra this patch adds
with respect to the one I posted,

Steve.