On Sat, Dec 08, 2012 at 07:29:21AM -0500, Christoph Hellwig wrote:
> On Tue, Dec 04, 2012 at 05:18:04PM -0600, Mark Tinguely wrote:
> > Not every segment in a multi-segment buffer is dirty in a
> > transaction and they will not be outputted. The assert in
> > xfs_buf_item_format_segment() that checks for the at least
> > one chunk of data in the segment to be used is not necessary
> > true for multi-segmented buffers.
> >
> > Signed-off-by: Mark Tinguely <tinguely@xxxxxxx>
> > ---
> > fs/xfs/xfs_buf_item.c | 20 +++++++++++++++-----
> > 1 file changed, 15 insertions(+), 5 deletions(-)
> >
> > Index: b/fs/xfs/xfs_buf_item.c
> > ===================================================================
> > --- a/fs/xfs/xfs_buf_item.c
> > +++ b/fs/xfs/xfs_buf_item.c
> > @@ -287,6 +287,17 @@ xfs_buf_item_format_segment(
> > */
> > base_size = offsetof(struct xfs_buf_log_format, blf_data_map) +
> > (blfp->blf_map_size * sizeof(blfp->blf_data_map[0]));
> > +
> > + nvecs = 0;
> > + first_bit = xfs_next_bit(blfp->blf_data_map, blfp->blf_map_size, 0);
> > + if (!(bip->bli_flags & XFS_BLI_STALE) && first_bit == -1) {
> > + /*
> > + * If the map is not be dirty in the transaction, mark
> > + * the size as zero and do not advance the vector pointer.
> > + */
> > + goto out;
> > + }
> > +
>
> I don't really like the style of this check. What's the problem of
> doing it this way:
>
> 1) fill out the first vecp
> 2) do the stale check as-is
> 3) handle the the first_bit == -1 case ala:
>
> if (first_bit == -1) {
> blfp->blf_size = 0;
> return vecp;
> }
Because if we have already written all the dirty regions and we have
a trailing clean buffer segment, vecp points past the memory
allocated for the vectors. Hence we can end up with memory
corruption if we write to vecp in this case.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
