Skip to contentSkip to conversation

xfs: Introduce a new function to find the desired type of offset from page cache

PATCH Superseded Jeff Liu 3 participants 7 messages Jul 26, 2012 – Jul 31, 2012 XFS filesystem

v5 of 8 Superseded Latest is v8 →

lore
Share & tools
lore

Jeff Liu

author
#0 lore raw

PATCH v5 2/4 xfs: Introduce a new function to find the desired type of offset from page cache

This function is called by xfs_seek_data() and xfs_seek_hole() to find the desired offset from page cache.

---
 fs/xfs/xfs_file.c |  203 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 203 insertions(+), 0 deletions(-)
fs/xfs/xfs_file.c +203 -0
index 98642cf..43f5e61 100644
@@ -36,6 +36,7 @@
 
 #include <linux/dcache.h>
 #include <linux/falloc.h>
+#include <linux/pagevec.h>
 
 static const struct vm_operations_struct xfs_file_vm_ops;
 
@@ -966,6 +967,208 @@ xfs_vm_page_mkwrite(
 	return block_page_mkwrite(vma, vmf, xfs_get_blocks);
 }
 
+/*
+ * This type is designed to indicate the type of offset we would like
+ * to search from page cache for either xfs_seek_data() or xfs_seek_hole().
+ */
+enum {
+	HOLE_OFF = 0,
+	DATA_OFF,
+};
+
+/*
+ * This routine is called to find out and return a data or hole offset
+ * from the page cache for unwritten extents according to the desired
+ * type for xfs_seek_data() or xfs_seek_hole().
+ *
+ * The argument offset is used to tell where we start to search from the
+ * page cache, and it will be filled with the desired type of offset if
+ * it was found, or it will keep unchanged.  map is used to figure out
+ * the end points of the range to lookup pages.
+ */
+STATIC bool
+xfs_find_get_desired_pgoff(
+	struct inode		*inode,
+	struct xfs_bmbt_irec	*map,
+	unsigned int		type,
+	loff_t			*offset)
+{
+	struct xfs_inode	*ip = XFS_I(inode);
+	struct xfs_mount	*mp = ip->i_mount;
+	struct pagevec		pvec;
+	pgoff_t			index;
+	pgoff_t			end;
+	loff_t			endoff;
+	loff_t			coff = *offset; /* current search offset */
+	bool			found = false;
+
+	pagevec_init(&pvec, 0);
+	index = XFS_FSB_TO_B(mp, XFS_B_TO_FSBT(mp, coff)) >> PAGE_CACHE_SHIFT;
+
+	/* The end offset to search for */
+	endoff = XFS_FSB_TO_B(mp, map->br_startoff + map->br_blockcount);
+	end = endoff >> PAGE_CACHE_SHIFT;
+
+	do {
+		unsigned int	i;
+		unsigned	nr_pages;
+		int		want = min_t(pgoff_t, end - index,
+					     (pgoff_t)PAGEVEC_SIZE - 1) + 1;
+
+		nr_pages = pagevec_lookup(&pvec, inode->i_mapping, index,
+					  want);
+		/*
+		 * No page mapped into given range.  If we are searching holes
+		 * and if this is the first time we got into the loop, it means
+		 * that the given offset is landed in a hole and return ture.
+		 *
+		 * If we have already stepped through some block buffers to find
+		 * holes but those buffers are all contains data, in this case,
+		 * the current search offset is already aligned to block buffer
+		 * unit boundary and pointed to the end of the last mapped page.
+		 * If it's location is less than the end range given for search,
+		 * that means there should be a hole between them, so return the
+		 * current search offset if we are searching hole.
+		 */
+		if (nr_pages == 0) {
+			if (type == HOLE_OFF) {
+				if (coff == *offset)
+					found = true;
+				if (coff < endoff) {
+					found = true;
+					*offset = coff;
+				}
+			}
+			/* Search data but nothing found */
+			break;
+		}
+
+		/*
+		 * At least we found one page.  If this the first time we step
+		 * into the loop, and if the first page index offset is greater
+		 * than the given search offset, a hole was found, return true
+		 * if we are searching holes.
+		 */
+		if ((type == HOLE_OFF) && (coff == *offset)) {
+			if (coff < pvec.pages[0]->index << PAGE_CACHE_SHIFT) {
+				found = true;
+				break;
+			}
+		}
+
+		for (i = 0; i < nr_pages; i++) {
+			struct page		*page = pvec.pages[i];
+			struct buffer_head	*bh;
+			struct buffer_head	*head;
+			xfs_fileoff_t		last;
+
+			/*
+			 * Page index is out of range, we need to deal with
+			 * hole search condition in paticular if that is the
+			 * desired type for the lookup.
+			 * stepping into the block buffer checkup, it probably
+			 * means that there is no page mapped at all in the
+			 * specified range to search, so we found a hole.
+			 * If we have already done some block buffer checking
+			 * and found one or more data buffers before, in this
+			 * case, the coff is already updated and it point to
+			 * the end of the last data buffer, so the left range
+			 * behind it might be a hole.  In either case, we will
+			 * return the coff to indicate a hole's location because
+			 * it must be greater than or equal to the search start.
+			 */
+			if (page->index > end) {
+				if (type == HOLE_OFF && coff < endoff) {
+					*offset = coff;
+					found = true;
+				}
+				goto out;
+			}
+
+			if (!trylock_page(page))
+				goto out;
+
+			if (!page_has_buffers(page)) {
+				unlock_page(page);
+				continue;
+			}
+
+			last = XFS_B_TO_FSBT(mp,
+					     page->index << PAGE_CACHE_SHIFT);
+			bh = head = page_buffers(page);
+			do {
+				off_t		lastoff = 0;
+
+				/*
+				 * The 1st block buffer offset in current page.
+				 */
+				lastoff = XFS_FSB_TO_B(mp, last);
+				/*
+				 * An extent in XFS_EXT_UNWRITTEN has disk
+				 * blocks already mapped to it, but no data
+				 * has been committed to them yet.  If it has
+				 * dirty data in the page cache it can be
+				 * identified by having BH_Unwritten set in
+				 * each buffers.  Also, the buffer head state
+				 * might be in BH_Uptodate too if the buffer
+				 * writeback procedure was fired, we have to
+				 * check it up as well.
+				 */
+				if (buffer_unwritten(bh) ||
+				    buffer_uptodate(bh)) {
+					/*
+					 * Found a data buffer and we are
+					 * searching data, great.
+					 */
+					if (type == DATA_OFF)
+						found = true;
+				} else {
+					/*
+					 * Nothing was found and we are
+					 * searching holes, great.
+					 */
+					if (type == HOLE_OFF)
+						found = true;
+				}
+				if (found) {
+					/*
+					 * Return if we found the desired
+					 * page offset.
+					 */
+					*offset = max_t(loff_t, coff, lastoff);
+					unlock_page(page);
+					goto out;
+				}
+				/*
+				 * We either searching data but nothing was
+				 * found, or searching hole but found a data
+				 * block buffer.  In either case, probably the
+				 * next block buffer is what we are desired,
+				 * so that we need to round up the current
+				 * offset to it.
+				 */
+				coff = round_up(lastoff + 1, bh->b_size);
+				last++;
+			} while ((bh = bh->b_this_page) != head);
+			unlock_page(page);
+		}
+
+		/*
+		 * If the number of returned pages less than our desired,
+		 * there should no more pages mapped, search done.
+		 */
+		if (nr_pages < want)
+			break;
+
+		index = pvec.pages[i - 1]->index + 1;
+		pagevec_release(&pvec);
+	} while (index < end);
+
+out:
+	pagevec_release(&pvec);
+	return found;
+}
+
 STATIC loff_t
 xfs_seek_data(
 	struct file		*file,
Signature · 6 lines
1.7.4.1

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
author
#1 lore raw
Quoted 210 lines · Jeff Liu

This function is called by xfs_seek_data() and xfs_seek_hole() to find the desired offset from page cache.

---
 fs/xfs/xfs_file.c |  203 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 203 insertions(+), 0 deletions(-)
fs/xfs/xfs_file.c +183 -0
index 98642cf..43f5e61 100644
@@ -36,6 +36,7 @@
 
 #include <linux/dcache.h>
 #include <linux/falloc.h>
+#include <linux/pagevec.h>
 
 static const struct vm_operations_struct xfs_file_vm_ops;
 
@@ -966,6 +967,208 @@ xfs_vm_page_mkwrite(
 	return block_page_mkwrite(vma, vmf, xfs_get_blocks);
 }
 
+/*
+ * This type is designed to indicate the type of offset we would like
+ * to search from page cache for either xfs_seek_data() or xfs_seek_hole().
+ */
+enum {
+	HOLE_OFF = 0,
+	DATA_OFF,
+};
+
+/*
+ * This routine is called to find out and return a data or hole offset
+ * from the page cache for unwritten extents according to the desired
+ * type for xfs_seek_data() or xfs_seek_hole().
+ *
+ * The argument offset is used to tell where we start to search from the
+ * page cache, and it will be filled with the desired type of offset if
+ * it was found, or it will keep unchanged.  map is used to figure out
+ * the end points of the range to lookup pages.
+ */
+STATIC bool
+xfs_find_get_desired_pgoff(
+	struct inode		*inode,
+	struct xfs_bmbt_irec	*map,
+	unsigned int		type,
+	loff_t			*offset)
+{
+	struct xfs_inode	*ip = XFS_I(inode);
+	struct xfs_mount	*mp = ip->i_mount;
+	struct pagevec		pvec;
+	pgoff_t			index;
+	pgoff_t			end;
+	loff_t			endoff;
+	loff_t			coff = *offset; /* current search offset */
+	bool			found = false;
+
+	pagevec_init(&pvec, 0);
+	index = XFS_FSB_TO_B(mp, XFS_B_TO_FSBT(mp, coff)) >> PAGE_CACHE_SHIFT;
+
+	/* The end offset to search for */
+	endoff = XFS_FSB_TO_B(mp, map->br_startoff + map->br_blockcount);
+	end = endoff >> PAGE_CACHE_SHIFT;
+
+	do {
+		unsigned int	i;
+		unsigned	nr_pages;
+		int		want = min_t(pgoff_t, end - index,
+					     (pgoff_t)PAGEVEC_SIZE - 1) + 1;
+
+		nr_pages = pagevec_lookup(&pvec, inode->i_mapping, index,
+					  want);
+		/*
+		 * No page mapped into given range.  If we are searching holes
+		 * and if this is the first time we got into the loop, it means
+		 * that the given offset is landed in a hole and return ture.
+		 *
+		 * If we have already stepped through some block buffers to find
+		 * holes but those buffers are all contains data, in this case,
+		 * the current search offset is already aligned to block buffer
+		 * unit boundary and pointed to the end of the last mapped page.
+		 * If it's location is less than the end range given for search,
+		 * that means there should be a hole between them, so return the
+		 * current search offset if we are searching hole.
+		 */
+		if (nr_pages == 0) {
+			if (type == HOLE_OFF) {
+				if (coff == *offset)
+					found = true;
+				if (coff < endoff) {
+					found = true;
+					*offset = coff;
+				}
+			}
+			/* Search data but nothing found */
+			break;
+		}
+
+		/*
+		 * At least we found one page.  If this the first time we step
+		 * into the loop, and if the first page index offset is greater
+		 * than the given search offset, a hole was found, return true
+		 * if we are searching holes.
+		 */
+		if ((type == HOLE_OFF) && (coff == *offset)) {
+			if (coff < pvec.pages[0]->index << PAGE_CACHE_SHIFT) {
+				found = true;
+				break;
+			}
+		}
+
+		for (i = 0; i < nr_pages; i++) {
+			struct page		*page = pvec.pages[i];
+			struct buffer_head	*bh;
+			struct buffer_head	*head;
+			xfs_fileoff_t		last;
+
+			/*
+			 * Page index is out of range, we need to deal with
+			 * hole search condition in paticular if that is the
+			 * desired type for the lookup.
+			 * stepping into the block buffer checkup, it probably
+			 * means that there is no page mapped at all in the
+			 * specified range to search, so we found a hole.
+			 * If we have already done some block buffer checking
+			 * and found one or more data buffers before, in this
+			 * case, the coff is already updated and it point to
+			 * the end of the last data buffer, so the left range
+			 * behind it might be a hole.  In either case, we will
+			 * return the coff to indicate a hole's location because
+			 * it must be greater than or equal to the search start.
+			 */
+			if (page->index > end) {
+				if (type == HOLE_OFF && coff < endoff) {
+					*offset = coff;
+					found = true;
+				}
+				goto out;
+			}
+
+			if (!trylock_page(page))
+				goto out;
+
+			if (!page_has_buffers(page)) {
+				unlock_page(page);
+				continue;
+			}
+
+			last = XFS_B_TO_FSBT(mp,
+					     page->index << PAGE_CACHE_SHIFT);
+			bh = head = page_buffers(page);
+			do {
+				off_t		lastoff = 0;
+
+				/*
+				 * The 1st block buffer offset in current page.
+				 */
+				lastoff = XFS_FSB_TO_B(mp, last);
+				/*
+				 * An extent in XFS_EXT_UNWRITTEN has disk
+				 * blocks already mapped to it, but no data
+				 * has been committed to them yet.  If it has
+				 * dirty data in the page cache it can be
+				 * identified by having BH_Unwritten set in
+				 * each buffers.  Also, the buffer head state
+				 * might be in BH_Uptodate too if the buffer
+				 * writeback procedure was fired, we have to
+				 * check it up as well.
+				 */
+				if (buffer_unwritten(bh) ||
+				    buffer_uptodate(bh)) {
+					/*
+					 * Found a data buffer and we are
+					 * searching data, great.
+					 */
+					if (type == DATA_OFF)
+						found = true;
+				} else {
+					/*
+					 * Nothing was found and we are
+					 * searching holes, great.
+					 */
+					if (type == HOLE_OFF)
+						found = true;
+				}
+				if (found) {
+					/*
+					 * Return if we found the desired
+					 * page offset.
+					 */
+					*offset = max_t(loff_t, coff, lastoff);
+					unlock_page(page);
+					goto out;
+				}
+				/*
+				 * We either searching data but nothing was
+				 * found, or searching hole but found a data
+				 * block buffer.  In either case, probably the
+				 * next block buffer is what we are desired,
+				 * so that we need to round up the current
+				 * offset to it.
+				 */
+				coff = round_up(lastoff + 1, bh->b_size);
+				last++;

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1161

Quoted 10 lines
fs/xfs/xfs_file.c +10 -0
+			} while ((bh = bh->b_this_page) != head);
+			unlock_page(page);
+		}
+
+		/*
+		 * If the number of returned pages less than our desired,
+		 * there should no more pages mapped, search done.
+		 */
+		if (nr_pages < want)
+			break;

Just found an issue here, should check the 'nr_pages < want' condition for searching hole, I'll re-send this patch set a little while. Sorry for the noise!

-Jeff

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1174

Quoted 13 lines
fs/xfs/xfs_file.c +10 -0
+
+		index = pvec.pages[i - 1]->index + 1;
+		pagevec_release(&pvec);
+	} while (index < end);
+
+out:
+	pagevec_release(&pvec);
+	return found;
+}
+
 STATIC loff_t
 xfs_seek_data(
 	struct file		*file,

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

author
#2 lore raw

PATCH v5 2/4 xfs: Introduce a new function to find the desired type of offset from page cache

In reply to Jeff Liu

This function is called by xfs_seek_data() and xfs_seek_hole() to find the desired offset from page cache.

---
 fs/xfs/xfs_file.c |  208 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 208 insertions(+), 0 deletions(-)
fs/xfs/xfs_file.c +208 -0
index 98642cf..69965a4 100644
@@ -36,6 +36,7 @@
 
 #include <linux/dcache.h>
 #include <linux/falloc.h>
+#include <linux/pagevec.h>
 
 static const struct vm_operations_struct xfs_file_vm_ops;
 
@@ -966,6 +967,213 @@ xfs_vm_page_mkwrite(
 	return block_page_mkwrite(vma, vmf, xfs_get_blocks);
 }
 
+/*
+ * This type is designed to indicate the type of offset we would like
+ * to search from page cache for either xfs_seek_data() or xfs_seek_hole().
+ */
+enum {
+	HOLE_OFF = 0,
+	DATA_OFF,
+};
+
+/*
+ * This routine is called to find out and return a data or hole offset
+ * from the page cache for unwritten extents according to the desired
+ * type for xfs_seek_data() or xfs_seek_hole().
+ *
+ * The argument offset is used to tell where we start to search from the
+ * page cache, and it will be filled with the desired type of offset if
+ * it was found, or it will keep unchanged.  map is used to figure out
+ * the end points of the range to lookup pages.
+ */
+STATIC bool
+xfs_find_get_desired_pgoff(
+	struct inode		*inode,
+	struct xfs_bmbt_irec	*map,
+	unsigned int		type,
+	loff_t			*offset)
+{
+	struct xfs_inode	*ip = XFS_I(inode);
+	struct xfs_mount	*mp = ip->i_mount;
+	struct pagevec		pvec;
+	pgoff_t			index;
+	pgoff_t			end;
+	loff_t			endoff;
+	loff_t			coff = *offset; /* current search offset */
+	bool			found = false;
+
+	pagevec_init(&pvec, 0);
+	index = XFS_FSB_TO_B(mp, XFS_B_TO_FSBT(mp, coff)) >> PAGE_CACHE_SHIFT;
+
+	/* The end offset to search for */
+	endoff = XFS_FSB_TO_B(mp, map->br_startoff + map->br_blockcount);
+	end = endoff >> PAGE_CACHE_SHIFT;
+
+	do {
+		unsigned int	i;
+		unsigned	nr_pages;
+		int		want = min_t(pgoff_t, end - index,
+					     (pgoff_t)PAGEVEC_SIZE - 1) + 1;
+
+		nr_pages = pagevec_lookup(&pvec, inode->i_mapping, index,
+					  want);
+		/*
+		 * No page mapped into given range.  If we are searching holes
+		 * and if this is the first time we got into the loop, it means
+		 * that the given offset is landed in a hole and return ture.
+		 *
+		 * If we have already stepped through some block buffers to find
+		 * holes but those buffers are all contains data, in this case,
+		 * the current search offset is already aligned to block buffer
+		 * unit boundary and pointed to the end of the last mapped page.
+		 * If it's location is less than the end range given for search,
+		 * that means there should be a hole between them, so return the
+		 * current search offset if we are searching hole.
+		 */
+		if (nr_pages == 0) {
+			if (type == HOLE_OFF) {
+				if (coff == *offset)
+					found = true;
+				if (coff < endoff) {
+					found = true;
+					*offset = coff;
+				}
+			}
+			/* Search data but nothing found */
+			break;
+		}
+
+		/*
+		 * At least we found one page.  If this the first time we step
+		 * into the loop, and if the first page index offset is greater
+		 * than the given search offset, a hole was found, return true
+		 * if we are searching holes.
+		 */
+		if ((type == HOLE_OFF) && (coff == *offset)) {
+			if (coff < pvec.pages[0]->index << PAGE_CACHE_SHIFT) {
+				found = true;
+				break;
+			}
+		}
+
+		for (i = 0; i < nr_pages; i++) {
+			struct page		*page = pvec.pages[i];
+			struct buffer_head	*bh;
+			struct buffer_head	*head;
+			xfs_fileoff_t		last;
+
+			/*
+			 * Page index is out of range, we need to deal with
+			 * hole search condition in paticular if that is the
+			 * desired type for the lookup.
+			 * stepping into the block buffer checkup, it probably
+			 * means that there is no page mapped at all in the
+			 * specified range to search, so we found a hole.
+			 * If we have already done some block buffer checking
+			 * and found one or more data buffers before, in this
+			 * case, the coff is already updated and it point to
+			 * the end of the last data buffer, so the left range
+			 * behind it might be a hole.  In either case, we will
+			 * return the coff to indicate a hole's location because
+			 * it must be greater than or equal to the search start.
+			 */
+			if (page->index > end) {
+				if (type == HOLE_OFF && coff < endoff) {
+					*offset = coff;
+					found = true;
+				}
+				goto out;
+			}
+
+			if (!trylock_page(page))
+				goto out;
+
+			if (!page_has_buffers(page)) {
+				unlock_page(page);
+				continue;
+			}
+
+			last = XFS_B_TO_FSBT(mp,
+					     page->index << PAGE_CACHE_SHIFT);
+			bh = head = page_buffers(page);
+			do {
+				off_t		lastoff = 0;
+
+				/*
+				 * The 1st block buffer offset in current page.
+				 */
+				lastoff = XFS_FSB_TO_B(mp, last);
+				/*
+				 * An extent in XFS_EXT_UNWRITTEN has disk
+				 * blocks already mapped to it, but no data
+				 * has been committed to them yet.  If it has
+				 * dirty data in the page cache it can be
+				 * identified by having BH_Unwritten set in
+				 * each buffers.  Also, the buffer head state
+				 * might be in BH_Uptodate too if the buffer
+				 * writeback procedure was fired, we have to
+				 * check it up as well.
+				 */
+				if (buffer_unwritten(bh) ||
+				    buffer_uptodate(bh)) {
+					/*
+					 * Found a data buffer and we are
+					 * searching data, great.
+					 */
+					if (type == DATA_OFF)
+						found = true;
+				} else {
+					/*
+					 * Nothing was found and we are
+					 * searching holes, great.
+					 */
+					if (type == HOLE_OFF)
+						found = true;
+				}
+				if (found) {
+					/*
+					 * Return if we found the desired
+					 * page offset.
+					 */
+					*offset = max_t(loff_t, coff, lastoff);
+					unlock_page(page);
+					goto out;
+				}
+				/*
+				 * We either searching data but nothing was
+				 * found, or searching hole but found a data
+				 * block buffer.  In either case, probably the
+				 * next block buffer is what we are desired,
+				 * so that we need to round up the current
+				 * offset to it.
+				 */
+				coff = round_up(lastoff + 1, bh->b_size);
+				last++;
+			} while ((bh = bh->b_this_page) != head);
+			unlock_page(page);
+		}
+
+		/*
+		 * If the number of returned pages less than our desired,
+		 * there should no more pages mapped, search done.
+		 */
+		if (nr_pages < want) {
+			if (type == HOLE_OFF) {
+				*offset = coff;
+				found = true;
+			}
+			break;
+		}
+
+		index = pvec.pages[i - 1]->index + 1;
+		pagevec_release(&pvec);
+	} while (index < end);
+
+out:
+	pagevec_release(&pvec);
+	return found;
+}
+
 STATIC loff_t
 xfs_seek_data(
 	struct file		*file,
Signature · 6 lines
1.7.4.1

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#3 lore raw
Quoted 4 lines · Jeff Liu

This function is called by xfs_seek_data() and xfs_seek_hole() to find the desired offset from page cache.

Hopefully, I am not being a pain....

I just noticed that if trylock() failed you return found==0.
Wouldn't it be safer/more correct to assume a page that failed a try_lock to be data?

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1036

Quoted 4 lines
fs/xfs/xfs_file.c +4 -0
+		if (nr_pages == 0) {
+			if (type == HOLE_OFF) {
+				if (coff == *offset)
+					found = true;

is this necessary? wouldn't the next test also cover the above condition?

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1041

Quoted 5 lines
fs/xfs/xfs_file.c +5 -0
+				if (coff<  endoff) {
+					found = true;
+					*offset = coff;
+				}
+			}

I like informative comments, but some are bit verbose. I will pick on this one:

+ /*
+ * Page index is out of range, we need to deal with
+ * hole search condition in paticular if that is the
+ * desired type for the lookup.
+ * stepping into the block buffer checkup, it probably
+ * means that there is no page mapped at all in the
+ * specified range to search, so we found a hole.
+ * If we have already done some block buffer checking
+ * and found one or more data buffers before, in this
+ * case, the coff is already updated and it point to
+ * the end of the last data buffer, so the left range
+ * behind it might be a hole. In either case, we will
+ * return the coff to indicate a hole's location because
+ * it must be greater than or equal to the search start.
+ */

just a crude simplification - maybe it is too terse:
/*
* coff is the current offset of the page being tested.
* If the next page index is beyond the extent of interest,
* then we are done searching with the data search is
* false and hole search is true at the last coff. */

For holes you are looking for (page->index != coff) for every page, but in a indirect way. It had me a little confused, but eventually I figured it out. I am not sure if a doing that comparison directly would overly complicate the data search path.

Good work.

--Mark.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

author
#4 lore raw
Quoted 8 lines · Mark Tinguely
Quoted 4 lines · Jeff Liu

This function is called by xfs_seek_data() and xfs_seek_hole() to find the desired offset from page cache.

Hopefully, I am not being a pain....

Never. :)

Quoted 4 lines

I just noticed that if trylock() failed you return found==0. Wouldn't it be safer/more correct to assume a page that failed a try_lock to be data?

I'm afraid to assume a page is data per lock failed will cause an inaccurate result, because it might be a hole.

Quoted 8 lines

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1036

Quoted 6 lines
fs/xfs/xfs_file.c +4 -0
+        if (nr_pages == 0) {
+            if (type == HOLE_OFF) {
+                if (coff == *offset)
+                    found = true;

is this necessary? wouldn't the next test also cover the above condition?

They are two different scenarios in this point as I have mentioned in comments, but they can be merged into one line, i.e, in either case, for searching holes, "*offset = coff and found = true".

Quoted 28 lines

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1041

Quoted 6 lines
fs/xfs/xfs_file.c +5 -0
+                if (coff<  endoff) {
+                    found = true;
+                    *offset = coff;
+                }
+            }

I like informative comments, but some are bit verbose. I will pick on this one:

fs/xfs/xfs_file.c +15 -0
+            /*
+             * Page index is out of range, we need to deal with
+             * hole search condition in paticular if that is the
+             * desired type for the lookup.
+             * stepping into the block buffer checkup, it probably
+             * means that there is no page mapped at all in the
+             * specified range to search, so we found a hole.
+             * If we have already done some block buffer checking
+             * and found one or more data buffers before, in this
+             * case, the coff is already updated and it point to
+             * the end of the last data buffer, so the left range
+             * behind it might be a hole.  In either case, we will
+             * return the coff to indicate a hole's location because
+             * it must be greater than or equal to the search start.
+             */
Quoted 7 lines

just a crude simplification - maybe it is too terse:
/*
* coff is the current offset of the page being tested.
* If the next page index is beyond the extent of interest,
* then we are done searching with the data search is
* false and hole search is true at the last coff. */

Exactly, thank you!

Quoted 6 lines

For holes you are looking for (page->index != coff) for every page, but in a indirect way. It had me a little confused, but eventually I figured it out. I am not sure if a doing that comparison directly would overly

complicate the data search path.

The current implements really looks complex, I will revise it combine with Dave's comments. Hopefully, those things would looks a bit simpler for my next try.

Thanks,
-Jeff

Quoted 9 lines

Good work.

--Mark.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

#5 lore raw
In reply to Jeff Liu
Quoted 7 lines · Jeff Liu

This function is called by xfs_seek_data() and xfs_seek_hole() to find the desired offset from page cache.

BTW, when you make a significant modification to a patch set, it is customary to drop existing reviewed-by tags on modified/new patches so that the reviewers know that it is new code and that they need to review at it again...

Quoted 9 lines

+/*
+ * This routine is called to find out and return a data or hole offset
+ * from the page cache for unwritten extents according to the desired
+ * type for xfs_seek_data() or xfs_seek_hole().
+ *
+ * The argument offset is used to tell where we start to search from the
+ * page cache, and it will be filled with the desired type of offset if
+ * it was found, or it will keep unchanged. map is used to figure out
+ * the end points of the range to lookup pages.

What's the return value? True for data, false for hole? Needs to be documented....

Quoted 9 lines

+ */

fs/xfs/xfs_file.c +8 -0
+STATIC bool
+xfs_find_get_desired_pgoff(
+	struct inode		*inode,
+	struct xfs_bmbt_irec	*map,
+	unsigned int		type,
+	loff_t			*offset)
+{
+	struct xfs_inode	*ip = XFS_I(inode);

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1006

Quoted 10 lines
fs/xfs/xfs_file.c +10 -0
+	struct xfs_mount	*mp = ip->i_mount;
+	struct pagevec		pvec;
+	pgoff_t			index;
+	pgoff_t			end;
+	loff_t			endoff;
+	loff_t			coff = *offset; /* current search offset */
+	bool			found = false;
+
+	pagevec_init(&pvec, 0);
+	index = XFS_FSB_TO_B(mp, XFS_B_TO_FSBT(mp, coff)) >> PAGE_CACHE_SHIFT;

That's a very complex way to find the page index. You are basically doing this for a 4k FSB/page size combination;

	index = ((coff >> 12) << 12) >> 12

For smaller block sizes, the one that matters is the PAGE_CACHE_SHIFT at the end. i.e:

	index = ((coff >> 9) << 9) >> 12
	      = coff >> 12

So why not just:

	index = coff >> PAGE_CACHE_SHIFT;

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1010

Quoted 4 lines
fs/xfs/xfs_file.c +4 -0
+
+	/* The end offset to search for */
+	endoff = XFS_FSB_TO_B(mp, map->br_startoff + map->br_blockcount);
+	end = endoff >> PAGE_CACHE_SHIFT;

I think there's an off-by-one here. Say we map fsb 0 for 2 blocks w/ 4k fsbs. that means byte offsets 0-8191 are in the range of the mapping. The above gives:

	endoff = (0 + 2) << 12 = 8192
	end = 8192 >> 12 = 2

so now we have start index 0, end index = 2. That's 3 pages, yes?

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1016

Quoted 5 lines
fs/xfs/xfs_file.c +5 -0
+	do {
+		unsigned int	i;
+		unsigned	nr_pages;
+		int		want = min_t(pgoff_t, end - index,
+					     (pgoff_t)PAGEVEC_SIZE - 1) + 1;

And that means here we do:

	want = min(2 - 0, 13) + 1
	     = 3.

So we are looking for 3 pages instead of only 2. And further down, the limits are (page->index > end) so this really does consider page index of 2 as part of the range asked for which it isn't....

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1023

Quoted 6 lines
fs/xfs/xfs_file.c +6 -0
+		nr_pages = pagevec_lookup(&pvec, inode->i_mapping, index,
+					  want);
+		/*
+		 * No page mapped into given range.  If we are searching holes
+		 * and if this is the first time we got into the loop, it means
+		 * that the given offset is landed in a hole and return ture.

true

Quoted 11 lines
fs/xfs/xfs_file.c +11 -0
+		 *
+		 * If we have already stepped through some block buffers to find
+		 * holes but those buffers are all contains data, in this case,
+		 * the current search offset is already aligned to block buffer
+		 * unit boundary and pointed to the end of the last mapped page.
+		 * If it's location is less than the end range given for search,
+		 * that means there should be a hole between them, so return the
+		 * current search offset if we are searching hole.
+		 */
+		if (nr_pages == 0) {
+			if (type == HOLE_OFF) {

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1044

Quoted 10 lines
fs/xfs/xfs_file.c +10 -0
+				if (coff == *offset)
+					found = true;
+				if (coff < endoff) {
+					found = true;
+					*offset = coff;
+				}
+			}
+			/* Search data but nothing found */
+			break;
+		}

I'd just write that as:

			/* data search found nothing */
			if (type == DATA_OFF)
				break;

			ASSERT(type == HOLE_OFF);
			if (coff == *offset || coff < endoff) {
				found = true;
				*offset = coff;
			}
			break;

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1052

Quoted 8 lines
fs/xfs/xfs_file.c +8 -0
+
+		/*
+		 * At least we found one page.  If this the first time we step
+		 * into the loop, and if the first page index offset is greater
+		 * than the given search offset, a hole was found, return true
+		 * if we are searching holes.
+		 */
+		if ((type == HOLE_OFF) && (coff == *offset)) {

No need for the extra () here....

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1053

Quoted 1 line
fs/xfs/xfs_file.c +1 -0
+			if (coff < pvec.pages[0]->index << PAGE_CACHE_SHIFT) {

... but that definitely needs them for clarity.

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1056

Quoted 4 lines
fs/xfs/xfs_file.c +4 -0
+				found = true;
+				break;
+			}
+		}
		if (type == HOLE_OFF && coff == *offset &&
		    coff < page_offset(pvec.pages[0])) {
			found = true;
			break;
		}

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1063

Quoted 6 lines
fs/xfs/xfs_file.c +6 -0
+
+		for (i = 0; i < nr_pages; i++) {
+			struct page		*page = pvec.pages[i];
+			struct buffer_head	*bh;
+			struct buffer_head	*head;
+			xfs_fileoff_t		last;

Indenting is starting to get excessive. Perhaps the page processing loop could be pushed into it's own function....

Quoted 6 lines
fs/xfs/xfs_file.c +6 -0
+
+			/*
+			 * Page index is out of range, we need to deal with
+			 * hole search condition in paticular if that is the
+			 * desired type for the lookup.
+			 * stepping into the block buffer checkup, it probably

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1079

Quoted 10 lines
fs/xfs/xfs_file.c +10 -0
+			 * means that there is no page mapped at all in the
+			 * specified range to search, so we found a hole.
+			 * If we have already done some block buffer checking
+			 * and found one or more data buffers before, in this
+			 * case, the coff is already updated and it point to
+			 * the end of the last data buffer, so the left range
+			 * behind it might be a hole.  In either case, we will
+			 * return the coff to indicate a hole's location because
+			 * it must be greater than or equal to the search start.
+			 */

The comment could do with some work - it seems to be very verbose fo the amount of information it conveys. Perhaps:

/*
* If the page index is out of range, then it means we
* found a hole. If we have already found some data
* buffers, we need to update the start location of the
* hole before returning.
*/

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1089

Quoted 10 lines
fs/xfs/xfs_file.c +10 -0
+			if (page->index > end) {
+				if (type == HOLE_OFF && coff < endoff) {
+					*offset = coff;
+					found = true;
+				}
+				goto out;
+			}
+
+			if (!trylock_page(page))
+				goto out;

Just lock the page - there is no deadlock we need to avoid and we really need to know the state of the page to be accurate. Also, once the page is locked you need to check against page->index again, as we could be racing with memory reclaim or truncate here. It is also worth checking page->mapping is still correctly set, too, for the same reason.

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1097

Quoted 8 lines
fs/xfs/xfs_file.c +8 -0
+
+			if (!page_has_buffers(page)) {
+				unlock_page(page);
+				continue;
+			}
+
+			last = XFS_B_TO_FSBT(mp,
+					     page->index << PAGE_CACHE_SHIFT);

Why calculate the last offset in FSB?

			last = page_offset(page);

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1100

Quoted 3 lines
fs/xfs/xfs_file.c +3 -0
+			bh = head = page_buffers(page);
+			do {
+				off_t		lastoff = 0;
				off_t	lastoff = last;

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1116

Quoted 11 lines
fs/xfs/xfs_file.c +11 -0
+				/*
+				 * An extent in XFS_EXT_UNWRITTEN has disk
+				 * blocks already mapped to it, but no data
+				 * has been committed to them yet.  If it has
+				 * dirty data in the page cache it can be
+				 * identified by having BH_Unwritten set in
+				 * each buffers.  Also, the buffer head state
+				 * might be in BH_Uptodate too if the buffer
+				 * writeback procedure was fired, we have to
+				 * check it up as well.
+				 */

buffer_uptodate() is also set in .commit_write when a page is fully written, and canbe set on read when mpage_readpages() gets confused and falls back to block_read_full_page. So that flag is no necessarily an indicator of dirty/writeback data. It is an indicator of data being present in the page cache, though, that may not be on disk. Hence the comment might be better written as:

/*
* Unwritten extents that have data in the page
* cache covering them can be identified by the
* buffer_unwritten() state flag. Pages with
* multiple buffers might have a mix of holes,
* data and unwritten extents - any buffer with
* valid data in it should have the
* buffer_uptodate(bh) flag set on it. */

Quoted 14 lines
fs/xfs/xfs_file.c +12 -0
+				if (buffer_unwritten(bh) ||
+				    buffer_uptodate(bh)) {
+					/*
+					 * Found a data buffer and we are
+					 * searching data, great.
+					 */
+					if (type == DATA_OFF)
+						found = true;
 
 
+				} else {
+					/*
+					 * Nothing was found and we are
+					 * searching holes, great.

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1138

Quoted 10 lines
fs/xfs/xfs_file.c +10 -0
+					 */
+					if (type == HOLE_OFF)
+						found = true;
+				}
+				if (found) {
+					/*
+					 * Return if we found the desired
+					 * page offset.
+					 */
+					*offset = max_t(loff_t, coff, lastoff);

When would lastoff < coff be true?

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1150

Quoted 12 lines
fs/xfs/xfs_file.c +12 -0
+					unlock_page(page);
+					goto out;
+				}
+				/*
+				 * We either searching data but nothing was
+				 * found, or searching hole but found a data
+				 * block buffer.  In either case, probably the
+				 * next block buffer is what we are desired,
+				 * so that we need to round up the current
+				 * offset to it.
+				 */
+				coff = round_up(lastoff + 1, bh->b_size);

I don't see the point of this round_up call. It's just a complex way of saying:

				coff = lastoff + bh->b_size;

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1151

Quoted 1 line
fs/xfs/xfs_file.c +1 -0
+				last++;
				last += bh->b_size;

So by the time we get to this buffer loop we will always update coff, or return lastoff which is updated at the start of the loop. So why do we even need last and lastoff? can't you just use coff?

i.e. you are doing this:

				last = offset
				do {
					lastoff = last;
					....
					if (found)
						return lastoff
					...
					coff = lastoff + bh->b_size;
					last += bh->b_size
				} while()

Which is simply:

				coff = offset
				do {
					....
					if (found)
						return coff
					...
					coff += bh->b_size
				} while()
Quoted 37 lines
fs/xfs/xfs_file.c +20 -0
+			} while ((bh = bh->b_this_page) != head);
+			unlock_page(page);
+		}
+
+		/*
+		 * If the number of returned pages less than our desired,
+		 * there should no more pages mapped, search done.
+		 */
+		if (nr_pages < want)
+			break;
+
+		index = pvec.pages[i - 1]->index + 1;
+		pagevec_release(&pvec);
+	} while (index < end);
+
+out:
+	pagevec_release(&pvec);
+	return found;
+}
+
 STATIC loff_t
 xfs_seek_data(
 	struct file		*file,
Signature · 11 lines
1.7.4.1

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

-- 
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.
Signature · 7 lines
Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
author
#6 lore raw
Quoted 4 lines · Dave Chinner
Quoted 3 lines · Jeff Liu

This function is called by xfs_seek_data() and xfs_seek_hole() to find the desired offset from page cache.

Quoted 8 lines
Quoted 4 lines

BTW, when you make a significant modification to a patch set, it is customary to drop existing reviewed-by tags on modified/new patches so that the reviewers know that it is new code and that they need to review at it again...

I'll take care of it, thanks.

Quoted 10 lines
Quoted 10 lines

+/*
+ * This routine is called to find out and return a data or hole offset
+ * from the page cache for unwritten extents according to the desired
+ * type for xfs_seek_data() or xfs_seek_hole().
+ *
+ * The argument offset is used to tell where we start to search from the
+ * page cache, and it will be filled with the desired type of offset if
+ * it was found, or it will keep unchanged. map is used to figure out
+ * the end points of the range to lookup pages.

Quoted 2 lines

What's the return value? True for data, false for hole? Needs to be documented....

True if an offset of the desired type was found, or false, the fix will be reflected in the next post.

Quoted 28 lines
Quoted 10 lines

+ */

fs/xfs/xfs_file.c +8 -0
+STATIC bool
+xfs_find_get_desired_pgoff(
+	struct inode		*inode,
+	struct xfs_bmbt_irec	*map,
+	unsigned int		type,
+	loff_t			*offset)
+{
+	struct xfs_inode	*ip = XFS_I(inode);

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1006

Quoted 10 lines
fs/xfs/xfs_file.c +10 -0
+	struct xfs_mount	*mp = ip->i_mount;
+	struct pagevec		pvec;
+	pgoff_t			index;
+	pgoff_t			end;
+	loff_t			endoff;
+	loff_t			coff = *offset; /* current search offset */
+	bool			found = false;
+
+	pagevec_init(&pvec, 0);
+	index = XFS_FSB_TO_B(mp, XFS_B_TO_FSBT(mp, coff)) >> PAGE_CACHE_SHIFT;

That's a very complex way to find the page index. You are basically doing this for a 4k FSB/page size combination;

	index = ((coff >> 12) << 12) >> 12

For smaller block sizes, the one that matters is the PAGE_CACHE_SHIFT at the end. i.e:

Quoted 6 lines
	index = ((coff >> 9) << 9) >> 12
	      = coff >> 12

So why not just:

	index = coff >> PAGE_CACHE_SHIFT;

Yes, I was silly to do so much redundant conversions. :(

Quoted 21 lines

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1010

Quoted 5 lines
fs/xfs/xfs_file.c +4 -0
+
+	/* The end offset to search for */
+	endoff = XFS_FSB_TO_B(mp, map->br_startoff + map->br_blockcount);
+	end = endoff >> PAGE_CACHE_SHIFT;

I think there's an off-by-one here. Say we map fsb 0 for 2 blocks w/ 4k fsbs. that means byte offsets 0-8191 are in the range of the mapping. The above gives:

	endoff = (0 + 2) << 12 = 8192
	end = 8192 >> 12 = 2

so now we have start index 0, end index = 2. That's 3 pages, yes?

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1016

Quoted 5 lines
fs/xfs/xfs_file.c +5 -0
+	do {
+		unsigned int	i;
+		unsigned	nr_pages;
+		int		want = min_t(pgoff_t, end - index,
+					     (pgoff_t)PAGEVEC_SIZE - 1) + 1;

And that means here we do:

Quoted 6 lines
	want = min(2 - 0, 13) + 1
	     = 3.

So we are looking for 3 pages instead of only 2. And further down, the limits are (page->index > end) so this really does consider page index of 2 as part of the range asked for which it isn't....

Exactly!

Quoted 31 lines

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1023

Quoted 7 lines
fs/xfs/xfs_file.c +6 -0
+		nr_pages = pagevec_lookup(&pvec, inode->i_mapping, index,
+					  want);
+		/*
+		 * No page mapped into given range.  If we are searching holes
+		 * and if this is the first time we got into the loop, it means
+		 * that the given offset is landed in a hole and return ture.

true

Quoted 11 lines
fs/xfs/xfs_file.c +11 -0
+		 *
+		 * If we have already stepped through some block buffers to find
+		 * holes but those buffers are all contains data, in this case,
+		 * the current search offset is already aligned to block buffer
+		 * unit boundary and pointed to the end of the last mapped page.
+		 * If it's location is less than the end range given for search,
+		 * that means there should be a hole between them, so return the
+		 * current search offset if we are searching hole.
+		 */
+		if (nr_pages == 0) {
+			if (type == HOLE_OFF) {

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1044

Quoted 10 lines
fs/xfs/xfs_file.c +10 -0
+				if (coff == *offset)
+					found = true;
+				if (coff < endoff) {
+					found = true;
+					*offset = coff;
+				}
+			}
+			/* Search data but nothing found */
+			break;
+		}

I'd just write that as:

Quoted 3 lines
			/* data search found nothing */
			if (type == DATA_OFF)
				break;

Make sense to break earlier for searching data.

Quoted 2 lines
			ASSERT(type == HOLE_OFF);

I wonder if we really need to assert or not, as this helper only used to lookup either data or hole.

Quoted 19 lines
			if (coff == *offset || coff < endoff) {
				found = true;
				*offset = coff;
			}
			break;

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1052

Quoted 8 lines
fs/xfs/xfs_file.c +8 -0
+
+		/*
+		 * At least we found one page.  If this the first time we step
+		 * into the loop, and if the first page index offset is greater
+		 * than the given search offset, a hole was found, return true
+		 * if we are searching holes.
+		 */
+		if ((type == HOLE_OFF) && (coff == *offset)) {

No need for the extra () here....

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1053

Quoted 1 line
fs/xfs/xfs_file.c +1 -0
+			if (coff < pvec.pages[0]->index << PAGE_CACHE_SHIFT) {

... but that definitely needs them for clarity.

Quoted 9 lines

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1056

Quoted 4 lines
fs/xfs/xfs_file.c +4 -0
+				found = true;
+				break;
+			}
+		}
		if (type == HOLE_OFF && coff == *offset &&
		    coff < page_offset(pvec.pages[0])) {
			found = true;
			break;
		}

Ok.

Quoted 8 lines

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1063

Quoted 6 lines
fs/xfs/xfs_file.c +6 -0
+
+		for (i = 0; i < nr_pages; i++) {
+			struct page		*page = pvec.pages[i];
+			struct buffer_head	*bh;
+			struct buffer_head	*head;
+			xfs_fileoff_t		last;

Indenting is starting to get excessive. Perhaps the page processing loop could be pushed into it's own function....

I'll try to isolate it in an individual function.

Quoted 20 lines
Quoted 7 lines
fs/xfs/xfs_file.c +6 -0
+
+			/*
+			 * Page index is out of range, we need to deal with
+			 * hole search condition in paticular if that is the
+			 * desired type for the lookup.
+			 * stepping into the block buffer checkup, it probably

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1079

Quoted 10 lines
fs/xfs/xfs_file.c +10 -0
+			 * means that there is no page mapped at all in the
+			 * specified range to search, so we found a hole.
+			 * If we have already done some block buffer checking
+			 * and found one or more data buffers before, in this
+			 * case, the coff is already updated and it point to
+			 * the end of the last data buffer, so the left range
+			 * behind it might be a hole.  In either case, we will
+			 * return the coff to indicate a hole's location because
+			 * it must be greater than or equal to the search start.
+			 */

The comment could do with some work - it seems to be very verbose fo the amount of information it conveys. Perhaps:

Quoted 6 lines

/*
* If the page index is out of range, then it means we
* found a hole. If we have already found some data
* buffers, we need to update the start location of the
* hole before returning.
*/

Will fix it combine with Mark's comments.

Quoted 17 lines

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1089

Quoted 11 lines
fs/xfs/xfs_file.c +10 -0
+			if (page->index > end) {
+				if (type == HOLE_OFF && coff < endoff) {
+					*offset = coff;
+					found = true;
+				}
+				goto out;
+			}
+
+			if (!trylock_page(page))
+				goto out;

Just lock the page - there is no deadlock we need to avoid and we really need to know the state of the page to be accurate. Also, once the page is locked you need to check against page->index again, as we could be racing with memory reclaim or truncate here. It is also worth checking page->mapping is still correctly set, too, for the same reason.

Thanks for the teaching! I was not aware of memory reclaim/truncate procedures which are behind the scene.

Quoted 12 lines

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1097

Quoted 10 lines
fs/xfs/xfs_file.c +8 -0
+
+			if (!page_has_buffers(page)) {
+				unlock_page(page);
+				continue;
+			}
+
+			last = XFS_B_TO_FSBT(mp,
+					     page->index << PAGE_CACHE_SHIFT);

Why calculate the last offset in FSB?

Quoted 1 line
			last = page_offset(page);

well... Duh. :)

Quoted 24 lines

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1100

Quoted 4 lines
fs/xfs/xfs_file.c +3 -0
+			bh = head = page_buffers(page);
+			do {
+				off_t		lastoff = 0;
				off_t	lastoff = last;

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1116

Quoted 11 lines
fs/xfs/xfs_file.c +11 -0
+				/*
+				 * An extent in XFS_EXT_UNWRITTEN has disk
+				 * blocks already mapped to it, but no data
+				 * has been committed to them yet.  If it has
+				 * dirty data in the page cache it can be
+				 * identified by having BH_Unwritten set in
+				 * each buffers.  Also, the buffer head state
+				 * might be in BH_Uptodate too if the buffer
+				 * writeback procedure was fired, we have to
+				 * check it up as well.
+				 */

buffer_uptodate() is also set in .commit_write when a page is fully written, and canbe set on read when mpage_readpages() gets confused and falls back to block_read_full_page. So that flag is no necessarily an indicator of dirty/writeback data. It is an indicator of data being present in the page cache, though, that may not be on disk. Hence the comment might be better written as:

Quoted 9 lines

/*
* Unwritten extents that have data in the page
* cache covering them can be identified by the
* buffer_unwritten() state flag. Pages with
* multiple buffers might have a mix of holes,
* data and unwritten extents - any buffer with
* valid data in it should have the
* buffer_uptodate(bh) flag set on it. */

Definitely, I'll change it accordingly.

Quoted 10 lines
Quoted 9 lines
fs/xfs/xfs_file.c +8 -0
+				if (buffer_unwritten(bh) ||
+				    buffer_uptodate(bh)) {
+					/*
+					 * Found a data buffer and we are
+					 * searching data, great.
+					 */
+					if (type == DATA_OFF)
+						found = true;
Quoted 15 lines
Quoted 4 lines
fs/xfs/xfs_file.c +4 -0
+				} else {
+					/*
+					 * Nothing was found and we are
+					 * searching holes, great.

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1138

Quoted 10 lines
fs/xfs/xfs_file.c +10 -0
+					 */
+					if (type == HOLE_OFF)
+						found = true;
+				}
+				if (found) {
+					/*
+					 * Return if we found the desired
+					 * page offset.
+					 */
+					*offset = max_t(loff_t, coff, lastoff);

When would lastoff < coff be true?

If this is the first time we stepped to here for searching data, and a data buffer was found. The 'coff' might be greater than the 'lastoff', assuming 'coff = *offset = 4097',
but the data buffer offset(lastoff) located at 4096, in this case, we need to return the maximum value.

Quoted 15 lines

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1150

Quoted 13 lines
fs/xfs/xfs_file.c +12 -0
+					unlock_page(page);
+					goto out;
+				}
+				/*
+				 * We either searching data but nothing was
+				 * found, or searching hole but found a data
+				 * block buffer.  In either case, probably the
+				 * next block buffer is what we are desired,
+				 * so that we need to round up the current
+				 * offset to it.
+				 */
+				coff = round_up(lastoff + 1, bh->b_size);

I don't see the point of this round_up call. It's just a complex way of saying:

Quoted 1 line
				coff = lastoff + bh->b_size;

ok, will fix it.

Quoted 7 lines

fs/xfs/xfs_file.c xfs_vm_page_mkwrite( · line 1151

Quoted 2 lines
fs/xfs/xfs_file.c +1 -0
+				last++;
				last += bh->b_size;

So by the time we get to this buffer loop we will always update coff, or return lastoff which is updated at the start of the loop. So why do we even need last and lastoff? can't you just use coff?

Similar to above.
If this is the first time for searching data and we found a data buffer.
'coff' does not updated in this stage, it still point to '*offset', but the 'lastoff' is point to the current data buffer offset.

So 'coff' might be greater than, less than or equtal to 'lastoff'.
If it is greater than or equtal to the lastoff, return 'coff' is ok. However, if it is less than the 'lastoff', we should return 'lastoff' instead.

Quoted 42 lines

i.e. you are doing this:

				last = offset
				do {
					lastoff = last;
					....
					if (found)
						return lastoff
					...
					coff = lastoff + bh->b_size;
					last += bh->b_size
				} while()

Which is simply:

				coff = offset
				do {
					....
					if (found)
						return coff
					...
					coff += bh->b_size
				} while()
Quoted 17 lines
fs/xfs/xfs_file.c +17 -0
+			} while ((bh = bh->b_this_page) != head);
+			unlock_page(page);
+		}
+
+		/*
+		 * If the number of returned pages less than our desired,
+		 * there should no more pages mapped, search done.
+		 */
+		if (nr_pages < want)
+			break;
+
+		index = pvec.pages[i - 1]->index + 1;
+		pagevec_release(&pvec);
+	} while (index < end);
+
+out:
+	pagevec_release(&pvec);
Quoted 8 lines
Quoted 8 lines
fs/xfs/xfs_file.c +3 -0
+	return found;
+}
+
  STATIC loff_t
  xfs_seek_data(
  	struct file		*file,
Signature · 1 line
1.7.4.1

Thanks,
-Jeff

Quoted 11 lines
Quoted 11 lines

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

Signature · 3 lines
This message has been scanned for viruses and
dangerous content by MailScanner, and is
believed to be clean.

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs