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

Christoph Hellwig
2011-04-12T00:06:21Z

> 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
spot.

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.