[Top] [All Lists]

Re: [PATCH] xfs: reset buffer pointers before freeing them

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfs: reset buffer pointers before freeing them
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Mon, 11 Apr 2011 20:09:39 -0400
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1302491405-2290-1-git-send-email-david@xxxxxxxxxxxxx>
References: <1302491405-2290-1-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
>                       error = xlog_bread_noalign(log, ealign, sectbb, bp);
> -                     if (error)
> -                             break;
> -                     error = XFS_BUF_SET_PTR(bp, offset, bufblks);
> +                     /* must reset buffer pointer even on error */
> +                     error2 = XFS_BUF_SET_PTR(bp, offset, bufblks);

This seems to be incorrect both in the original and your version.
>From all that I can see XFS_BUF_SET_PTR wants a byte count while bufblks
is a sector count, e.g. this should be:

                        error2 = XFS_BUF_SET_PTR(bp, offset, BBTOB(bufblks);

While at I think this mess with the buffer virtual mapping, read into it
and set it back mess needs to go into a single helper instead of beeing
duplicated three times.  By having named arguments with proper types
bugs like the existing one above should also be much more obvious to

As a follow on patch to that I think we could also get rid of all the
vmap manipultions entirely, and just specify and I/O offset and length
manually.  The only thing required for that is a number of pages
to skip at the beggining of the buffer from the log recovery code to
_xfs_buf_ioapply, either by passing it down procedurally, or by adding
a new filed to struct xfs_buf.  In fact just relaxing the b_offset
semantics to allow offsets larger than a page only in the I/O path would
do it, but I'm not sure that version helps long-term maintenance.

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