Skip to contentSkip to conversation

Fix up xfs_buf_associate_memory()

PATCH In review Lachlan McIlroy 2 participants 3 messages Nov 23, 2007 – Nov 26, 2007

lore
Share & tools
lore

Lachlan McIlroy

author
#0 lore raw

PATCH 1/1 Fix up xfs_buf_associate_memory()

Fixed a few bugs in xfs_buf_associate_memory() including:

- calculation of 'page_count' was incorrect as it did not consider the offset of 'mem' into the first page. The logic to bump 'page_count' didn't work if 'len' was <= PAGE_CACHE_SIZE (ie offset = 3k, len = 2k).
- setting b_buffer_length to 'len' is incorrect if 'offset' is > 0. Set it to the total length of the buffer.
- I suspect that passing a non-aligned address into mem_to_page() for the first page may have been causing issues - don't know but just tidy up that code anyway.

These fixes prevent an data corruption issue that can occur during log replay.

Lachlan

xfs/linux-2.6/xfs_buf.c renamed from xfs/linux-2.6/xfs_buf.c_1.247 +10 -17
@@ -726,14 +726,14 @@ xfs_buf_associate_memory(
 	int			rval;
 	int			i = 0;
 	size_t			ptr;
-	size_t			end, end_cur;
+	size_t			buflen;
 	off_t			offset;
 	int			page_count;
 
-	page_count = PAGE_CACHE_ALIGN(len) >> PAGE_CACHE_SHIFT;
-	offset = (off_t) mem - ((off_t)mem & PAGE_CACHE_MASK);
-	if (offset && (len > PAGE_CACHE_SIZE))
-		page_count++;
+	ptr = (size_t) mem & PAGE_CACHE_MASK;
+	offset = (off_t) mem - (off_t) ptr;
+	buflen = PAGE_CACHE_ALIGN(len + offset);
+	page_count = buflen >> PAGE_CACHE_SHIFT;
 
 	/* Free any previous set of page pointers */
 	if (bp->b_pages)

@@ -747,22 +747,15 @@ xfs_buf_associate_memory(
 		return rval;
 
 	bp->b_offset = offset;
-	ptr = (size_t) mem & PAGE_CACHE_MASK;
-	end = PAGE_CACHE_ALIGN((size_t) mem + len);
-	end_cur = end;
-	/* set up first page */
-	bp->b_pages[0] = mem_to_page(mem);
-
-	ptr += PAGE_CACHE_SIZE;
-	bp->b_page_count = ++i;
-	while (ptr < end) {
-		bp->b_pages[i] = mem_to_page((void *)ptr);
-		bp->b_page_count = ++i;
+
+	while (i < bp->b_page_count) {
+		bp->b_pages[i++] = mem_to_page((void *)ptr);
 		ptr += PAGE_CACHE_SIZE;
 	}
 	bp->b_locked = 0;
 
-	bp->b_count_desired = bp->b_buffer_length = len;
+	bp->b_count_desired = len;
+	bp->b_buffer_length = buflen;
 	bp->b_flags |= XBF_MAPPED;
 
 	return 0;
#1 lore raw
Quoted 13 lines · Lachlan McIlroy

Fixed a few bugs in xfs_buf_associate_memory() including:

- calculation of 'page_count' was incorrect as it did not consider the offset of 'mem' into the first page. The logic to bump 'page_count' didn't work if 'len' was <= PAGE_CACHE_SIZE (ie offset = 3k, len = 2k).
- setting b_buffer_length to 'len' is incorrect if 'offset' is > 0. Set it to the total length of the buffer.
- I suspect that passing a non-aligned address into mem_to_page() for the first page may have been causing issues - don't know but just tidy up that code anyway.

Quoted 2 lines

These fixes prevent an data corruption issue that can occur during log replay.

Last time I tried to clean up this gem everything went bezerk, so be aware :)

xfs/linux-2.6/xfs_buf.c renamed from xfs/linux-2.6/xfs_buf.c_1.247 +3 -0
@@ -726,14 +726,14 @@ xfs_buf_associate_memory(
 	int			rval;
 	int			i = 0;
 	size_t			ptr;
+	size_t			buflen;
 	off_t			offset;
 	int			page_count;
 
+	ptr = (size_t) mem & PAGE_CACHE_MASK;
+	offset = (off_t) mem - (off_t) ptr;

Casting pointers to size_t or off_t makes little sense, these should be unsigned long. And using a variable name of ptr is quite odd :)

+ while (i < bp->b_page_count) {
+ bp->b_pages[i++] = mem_to_page((void *)ptr);
ptr += PAGE_CACHE_SIZE;
}

This could be much cleaner written as:

	for (i = 0; i < bp->b_page_count; i++) {
		bp->b_pages[i] = mem_to_page((void *)ptr);
		ptr += PAGE_CACHE_SIZE;
	}
author
#2 lore raw
Quoted 34 lines · Christoph Hellwig
Quoted 13 lines · Lachlan McIlroy

Fixed a few bugs in xfs_buf_associate_memory() including:

- calculation of 'page_count' was incorrect as it did not consider the offset of 'mem' into the first page. The logic to bump 'page_count' didn't work if 'len' was <= PAGE_CACHE_SIZE (ie offset = 3k, len = 2k).
- setting b_buffer_length to 'len' is incorrect if 'offset' is > 0. Set it to the total length of the buffer.
- I suspect that passing a non-aligned address into mem_to_page() for the first page may have been causing issues - don't know but just tidy up that code anyway.

Quoted 2 lines

These fixes prevent an data corruption issue that can occur during log replay.

Last time I tried to clean up this gem everything went bezerk, so be aware :)

xfs/linux-2.6/xfs_buf.c renamed from xfs/linux-2.6/xfs_buf.c_1.247 +3 -0
@@ -726,14 +726,14 @@ xfs_buf_associate_memory(
 	int			rval;
 	int			i = 0;
 	size_t			ptr;
+	size_t			buflen;
 	off_t			offset;
 	int			page_count;
 
+	ptr = (size_t) mem & PAGE_CACHE_MASK;
+	offset = (off_t) mem - (off_t) ptr;
Quoted 2 lines

Casting pointers to size_t or off_t makes little sense, these should be unsigned long. And using a variable name of ptr is quite odd :)

I just left those as they were before the change. I'll tidy them too.

Quoted 6 lines
xfs/linux-2.6/xfs_buf.c renamed from xfs/linux-2.6/xfs_buf.c_1.247 +2 -0
+	while (i < bp->b_page_count) {
+		bp->b_pages[i++] = mem_to_page((void *)ptr);
 		ptr += PAGE_CACHE_SIZE;
 	}
Quoted 7 lines

This could be much cleaner written as:

	for (i = 0; i < bp->b_page_count; i++) {
		bp->b_pages[i] = mem_to_page((void *)ptr);
		ptr += PAGE_CACHE_SIZE;
	}

Fine with me.

Thanks.