On Mon, Aug 08, 2011 at 04:51:10PM +1000, Dave Chinner wrote:
> -xfs_buf_t *
> +struct xfs_buf *
> xfs_buf_get(
> - xfs_buftarg_t *target,/* target for buffer */
> + struct xfs_buftarg *target,/* target for buffer */
> xfs_off_t ioff, /* starting offset of range */
> size_t isize, /* length of range */
> xfs_buf_flags_t flags)
> {
> - xfs_buf_t *bp, *new_bp;
> + struct xfs_buf *bp;
> + struct xfs_buf *new_bp = NULL;
> int error = 0;
>
> - new_bp = xfs_buf_allocate(flags);
> - if (unlikely(!new_bp))
> - return NULL;
> -
> +again:
> bp = _xfs_buf_find(target, ioff, isize, flags, new_bp);
> - if (bp == new_bp) {
> + if (!(bp || new_bp)) {
> + new_bp = xfs_buf_allocate(flags);
> + if (unlikely(!new_bp))
> + return NULL;
> +
> + _xfs_buf_initialize(new_bp, target, ioff << BBSHIFT,
> + isize << BBSHIFT, flags);
> + goto again;
> + }
> +
> + if (!bp) {
> + xfs_buf_deallocate(new_bp);
> + return NULL;
> + } else if (bp == new_bp) {
> error = xfs_buf_allocate_memory(bp, flags);
> if (error)
> goto no_buffer;
> - } else {
> + } else if (new_bp)
> xfs_buf_deallocate(new_bp);
> - if (unlikely(bp == NULL))
> - return NULL;
> - }
The while loop looks a bit confusing to me. Why not unroll it similar
to how it's done for the VFS inode cache:
bp = _xfs_buf_find(target, ioff, isize, flags, new_bp);
if (bp)
goto found;
new_bp = xfs_buf_allocate(flags);
if (unlikely(!new_bp))
return NULL;
_xfs_buf_initialize(new_bp, target, ioff << BBSHIFT, isize << BBSHIFT,
flags);
bp = _xfs_buf_find(target, ioff, isize, flags, new_bp);
if (!bp) {
xfs_buf_deallocate(new_bp);
return NULL;
}
if (bp == new_bp) {
error = xfs_buf_allocate_memory(bp, flags);
if (error)
goto no_buffer;
}
found:
or even better make sure all buffer initialization is done either as
part of xfs_buf_find or the caller - although we'd need a variant
with caller locking if we want to go down the latter route. This
variant would be nessecary if we ever want to add shared/exclusive or
RCU locking for buffer lookup anyway.
There's also some other weird things here that should be cleaned up
when touching it:
- we should never be able to find a buffer with the wrong mapped
state, so this could be replaced by an assert
- there should be no need to re-assign b_bn and b_count_desired
when we find a buffer
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
