xfs
[Top] [All Lists]

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

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfs: reset buffer pointers before freeing them
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Wed, 13 Apr 2011 08:50:26 +1000
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20110412162237.GB22506@xxxxxxxxxxxxx>
References: <1302491405-2290-1-git-send-email-david@xxxxxxxxxxxxx> <20110412000939.GC29358@xxxxxxxxxxxxx> <20110412110028.GA31057@dastard> <20110412134543.GG31057@dastard> <20110412162237.GB22506@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
On Tue, Apr 12, 2011 at 12:22:37PM -0400, Christoph Hellwig wrote:
> On Tue, Apr 12, 2011 at 11:45:43PM +1000, Dave Chinner wrote:
> > Something like this?
> 
> Kinda.  A few comments below.
> 
> >  /*
> > + * Read at an offset into the buffer. Returns with the buffer in it's 
> > original
> > + * state regardless of the result of the read.
> > + */
> > +STATIC int
> > +xlog_bread_offset(
> > +   xlog_t          *log,
> > +   int             buf_offset,     /* offset into buffer */
> > +   int             buf_blks,       /* original buffer size */
> > +   xfs_daddr_t     blk_no,         /* block to read from */
> > +   int             nbblks,         /* blocks to read */
> > +   xfs_buf_t       *bp,
> > +   xfs_caddr_t     offset)
> > +{
> > +   xfs_caddr_t     orig_offset = XFS_BUF_PTR(bp);
> > +   int             error, error2;
> > +
> > +   error = XFS_BUF_SET_PTR(bp, offset + BBTOB(buf_offset), BBTOB(nbblks));
> 
> Passing in both offset and the buf_offset seems odd as they are only
> used together.  So just passing in one byte offset would make more
> sense to me.  Of course it's not actually an offset anyore, but virtual
> address, but we use that offset naming all around xlog_align/xlog_bread
> and callers.

Fair enough. I was struggling with the best way to pass and name
all the parameters. I just posted this before I went to bed to see
if this is what you had in mind....

> Also I don't think there is a need for the buf_blks argument.  The
> argument always comes directly from the xlog_get_bp number blocks
> argument, so the
> 
>       BBTOB(buf_blks)
> 
> can be replaced by simply using a copy of b_buffer_length taken at
> the beginning of the function.

Very true. My brain surely was not functioning properly ;)

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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