SEEK_DATA/SEEK_HOLE support

Dear developer,

Does anyone already worked on SEEK_DATA/SEEK_HOLE for XFS? I'd like to implement it if not. :)

Quoted 4 lines · Jeff Liu

Dear developer,

Does anyone already worked on SEEK_DATA/SEEK_HOLE for XFS? I'd like to implement it if not. :)

Dave mentioned he had a basic implementation, he might have some code that you can improve on.

Did we get consensus about the the semantics of them for unwritten extents? If we want them to be exact in the fact of unwritten extents that is going to be the most work, e.g. if we find an unwritten extent we'll then have to do a pagecache lookup and check if pages are dirty and in that case not treat them as a hole.

The rest of the implementation should be easy, e.g. doing something like the following pseudo-code which missed all the proper error codes and conversions from the lseek arguments to filesystem blocks and the required locking around it:

seek_hole()

{
	xfs_bmap_search_extents(ip, bno, XFS_DATA_FORK, &eof, &lastx, &got,
			&prev);

	for (;;) {
		if (eof) {
			/* past the file, nothing do here */
			return;
		}

		if (got.br_startoff > bno)
			/* found hole, return it */
			return;
		}

		if (++lastx == ifp->if_bytes / sizeof(xfs_bmbt_rec_t)) {
			/* reached EOF */
			return;
		}
		xfs_bmbt_get_all(xfs_iext_get_ext(ifp, lastx), &got);
	}
}

seek_data()

{
	xfs_bmap_search_extents(ip, bno, XFS_DATA_FORK, &eof, &lastx, &got,
			&prev);

	for (;;) {
		if (eof) {
			/* past the file, nothing do here */
			return;
		}

		/* next data extent */
		return got.br_startoff < bno ? bno : got.br_startof;
	}
}

IMHO, to avoid data loss in some user application like cp(1), for unwritten extents, we always need to check the pages status. Just as you mentioned above, return the map offset if pages are dirty for SEEK_DATA, or a hole found.

And the most important thing is of course adding QA for it. Josef already wrote an xfstests case that needs to be resurrected, compared against the latest Posix draft and if nessecary updated.

Quoted 4 lines · Jeff Liu

IMHO, to avoid data loss in some user application like cp(1), for unwritten extents, we always need to check the pages status. Just as you mentioned above, return the map offset if pages are dirty for SEEK_DATA, or a hole found.

I'd suggest to first implement the simple versions I schemed below, which would treat unwritten extents as data. That is sub-optimal, but a) safe and b) easy to implement. The second step would be to add probing for unwritten extents, which is even something we could do as a common helper routine shared by filesystems.

And the most important thing is of course adding QA for it. Josef already wrote an xfstests case that needs to be resurrected, compared against the latest Posix draft and if nessecary updated.

Quoted 11 lines · Christoph Hellwig

Quoted 4 lines · Jeff Liu

IMHO, to avoid data loss in some user application like cp(1), for unwritten extents, we always need to check the pages status. Just as you mentioned above, return the map offset if pages are dirty for SEEK_DATA, or a hole found.

I'd suggest to first implement the simple versions I schemed below, which would treat unwritten extents as data. That is sub-optimal, but a) safe and b) easy to implement. The second step would be to add probing for unwritten extents, which is even something we could do as a common helper routine shared by filesystems.

So I'll wait for Dave's patch become ready, and then continue to improve it if necessary.
In the meantime, I'll try to figure out how to add a helper which can be shared by all file systems for UNWRITTEN extents.

The lookup is pretty simple - if there's cached data over the unwritten range, then I'm considering it a data range. If there's no cached data over the unwritten extent, it's a hole. That makes the lookup simply a case of finding the first cached page in the unwritten extent.

It'll end up reading something like this:

	iomap = offset_to_extent(offset);
	first_index = extent_to_page_index(iomap);

	nr_found = pagevec_lookup(&pvec, inode->i_mapping, first_index, 1);
	if (!nr_found)
		break;

	offset = page->index << PAGECACHE_SHIFT;
	pagevec_release(&pvec);

	/* If we fell off the end of the extent lookup next extent */
	if (offset >= end_of_extent(iomap)) {
		offset = end_of_extent(iomap);
		goto next_extent;
	}

All the extent manipulations are pretty filesystem specific, so there's not much that can be extracted into generic helper, I think...

Quoted 24 lines · Dave Chinner

The lookup is pretty simple - if there's cached data over the unwritten range, then I'm considering it a data range. If there's no cached data over the unwritten extent, it's a hole. That makes the lookup simply a case of finding the first cached page in the unwritten extent.

It'll end up reading something like this:

	iomap = offset_to_extent(offset);
	first_index = extent_to_page_index(iomap);

	nr_found = pagevec_lookup(&pvec, inode->i_mapping, first_index, 1);
	if (!nr_found)
		break;

	offset = page->index << PAGECACHE_SHIFT;
	pagevec_release(&pvec);

	/* If we fell off the end of the extent lookup next extent */
	if (offset >= end_of_extent(iomap)) {
		offset = end_of_extent(iomap);
		goto next_extent;
	}

Quoted 3 lines

All the extent manipulations are pretty filesystem specific, so there's not much that can be extracted into generic helper, I think...

Actually pretty similar code will work just fine if you passt the start + len of the extents in (which we got from looking it up fs-specificly):

Note that we have to look for both dirty and writeback pages to make it safe.

Quoted 29 lines · Christoph Hellwig

Quoted 24 lines · Dave Chinner

The lookup is pretty simple - if there's cached data over the unwritten range, then I'm considering it a data range. If there's no cached data over the unwritten extent, it's a hole. That makes the lookup simply a case of finding the first cached page in the unwritten extent.

It'll end up reading something like this:

	iomap = offset_to_extent(offset);
	first_index = extent_to_page_index(iomap);

	nr_found = pagevec_lookup(&pvec, inode->i_mapping, first_index, 1);
	if (!nr_found)
		break;

	offset = page->index << PAGECACHE_SHIFT;
	pagevec_release(&pvec);

	/* If we fell off the end of the extent lookup next extent */
	if (offset >= end_of_extent(iomap)) {
		offset = end_of_extent(iomap);
		goto next_extent;
	}

Quoted 3 lines

All the extent manipulations are pretty filesystem specific, so there's not much that can be extracted into generic helper, I think...

Quoted 6 lines

Actually pretty similar code will work just fine if you passt the start + len of the extents in (which we got from looking it up fs-specificly):

Note that we have to look for both dirty and writeback pages to make it safe.

That will only work if you can prevent concurrent unwritten extent conversion from happening while we do the separate tag lookups on the range because it requires two radix tree tag lookups rather than just one index lookup. i.e. miss the dirty page because it went dirty->writeback during the dirty tag search, and miss the same page when doing the writeback lookup because it went writeback->clean very quickly due to IO completion.

So to stop that from happening, it requires that filesystems can exclude unwritten extent conversion from happening while a SEEK_HOLE/SEEK_DATA operation is in progress, and that the filesystem can safely do mapping tree lookups while providing that extent tree exclusion. I know that XFS has no problems here, but filesystems that use i_mutex for everything might be in trouble.

Besides, if we just look for pages in the cache over unwritten extents (i.e. someone has treated it as data already), then it can be done locklessly without having to worry about page state changes occurring concurrently...

From some backup utility's point of view, AFAICS, the benefits is,

Cheers,

Dave.

Quoted 53 lines · Dave Chinner

Quoted 29 lines · Christoph Hellwig

Quoted 24 lines · Dave Chinner

The lookup is pretty simple - if there's cached data over the unwritten range, then I'm considering it a data range. If there's no cached data over the unwritten extent, it's a hole. That makes the lookup simply a case of finding the first cached page in the unwritten extent.

It'll end up reading something like this:

	iomap = offset_to_extent(offset);
	first_index = extent_to_page_index(iomap);

	nr_found = pagevec_lookup(&pvec, inode->i_mapping, first_index, 1);
	if (!nr_found)
		break;

	offset = page->index << PAGECACHE_SHIFT;
	pagevec_release(&pvec);

	/* If we fell off the end of the extent lookup next extent */
	if (offset >= end_of_extent(iomap)) {
		offset = end_of_extent(iomap);
		goto next_extent;
	}

Quoted 3 lines

All the extent manipulations are pretty filesystem specific, so there's not much that can be extracted into generic helper, I think...

Quoted 6 lines

Actually pretty similar code will work just fine if you passt the start + len of the extents in (which we got from looking it up fs-specificly):

Note that we have to look for both dirty and writeback pages to make it safe.

That will only work if you can prevent concurrent unwritten extent conversion from happening while we do the separate tag lookups on the range because it requires two radix tree tag lookups rather than just one index lookup. i.e. miss the dirty page because it went dirty->writeback during the dirty tag search, and miss the same page when doing the writeback lookup because it went writeback->clean very quickly due to IO completion.

So to stop that from happening, it requires that filesystems can exclude unwritten extent conversion from happening while a SEEK_HOLE/SEEK_DATA operation is in progress, and that the filesystem can safely do mapping tree lookups while providing that extent tree exclusion. I know that XFS has no problems here, but filesystems that use i_mutex for everything might be in trouble.

Quoted 4 lines

Besides, if we just look for pages in the cache over unwritten extents (i.e. someone has treated it as data already), then it can be done locklessly without having to worry about page state changes occurring concurrently...

From some backup utility's point of view, AFAICS, the benefits is, utilites does not need to perform read/write for an unwritten extents without dirty or writeback pages, instead, just call lseek(2) to treat it as hole.

The issue is, is it worth to do that? i.e, exclude unwritten extent conversions when SEEK_XXX is ongoing.

Quoted 60 lines · Jeff Liu

Quoted 53 lines · Dave Chinner

Quoted 29 lines · Christoph Hellwig

Quoted 24 lines · Dave Chinner

The lookup is pretty simple - if there's cached data over the unwritten range, then I'm considering it a data range. If there's no cached data over the unwritten extent, it's a hole. That makes the lookup simply a case of finding the first cached page in the unwritten extent.

It'll end up reading something like this:

	iomap = offset_to_extent(offset);
	first_index = extent_to_page_index(iomap);

	nr_found = pagevec_lookup(&pvec, inode->i_mapping, first_index, 1);
	if (!nr_found)
		break;

	offset = page->index << PAGECACHE_SHIFT;
	pagevec_release(&pvec);

	/* If we fell off the end of the extent lookup next extent */
	if (offset >= end_of_extent(iomap)) {
		offset = end_of_extent(iomap);
		goto next_extent;
	}

Quoted 3 lines

All the extent manipulations are pretty filesystem specific, so there's not much that can be extracted into generic helper, I think...

Quoted 6 lines

Actually pretty similar code will work just fine if you passt the start + len of the extents in (which we got from looking it up fs-specificly):

Note that we have to look for both dirty and writeback pages to make it safe.

That will only work if you can prevent concurrent unwritten extent conversion from happening while we do the separate tag lookups on the range because it requires two radix tree tag lookups rather than just one index lookup. i.e. miss the dirty page because it went dirty->writeback during the dirty tag search, and miss the same page when doing the writeback lookup because it went writeback->clean very quickly due to IO completion.

So to stop that from happening, it requires that filesystems can exclude unwritten extent conversion from happening while a SEEK_HOLE/SEEK_DATA operation is in progress, and that the filesystem can safely do mapping tree lookups while providing that extent tree exclusion. I know that XFS has no problems here, but filesystems that use i_mutex for everything might be in trouble.

Quoted 4 lines

Besides, if we just look for pages in the cache over unwritten extents (i.e. someone has treated it as data already), then it can be done locklessly without having to worry about page state changes occurring concurrently...

Quoted 4 lines

From some backup utility's point of view, AFAICS, the benefits is, utilites does not need to perform read/write for an unwritten extents without dirty or writeback pages, instead, just call lseek(2) to treat it as hole.

Yes, that's a use case for SEEK_HOLE/DATA, but that has nothing to do with the filesystem implementation issues involved with page state changes. What we nee dto avoid is a page state change from causing a SEEK_XXX from missing a page it should have stopped the seek at because it changed state concurrently. A page changing from dirty to writeback is still a page containing DATA, so if we miss it in both the scan for dirty pages and for writeback pages, then the backup application will seek over data it should have backed up....

Quoted 2 lines

The issue is, is it worth to do that? i.e, exclude unwritten extent conversions when SEEK_XXX is ongoing.

It's a moot point for XFS - we need to hold the ILOCK shared while doing the extent tree walk, so we are going to hold off unwritten extent conversion as that requires the ILOCK exclusively...

Cheers,

Dave.

That will only work if you can prevent concurrent unwritten extent conversion from happening while we do the separate tag lookups on the range because it requires two radix tree tag lookups rather than just one index lookup. i.e. miss the dirty page because it went dirty->writeback during the dirty tag search, and miss the same page when doing the writeback lookup because it went writeback->clean very quickly due to IO completion.

So to stop that from happening, it requires that filesystems can exclude unwritten extent conversion from happening while a SEEK_HOLE/SEEK_DATA operation is in progress, and that the filesystem can safely do mapping tree lookups while providing that extent tree exclusion. I know that XFS has no problems here, but filesystems that use i_mutex for everything might be in trouble.

Besides, if we just look for pages in the cache over unwritten extents (i.e. someone has treated it as data already), then it can be done locklessly without having to worry about page state changes occurring concurrently...

Quoted 15 lines · On Mittwoch, 5. Oktober 2011 Dave Chinner wrote:

That will only work if you can prevent concurrent unwritten extent conversion from happening while we do the separate tag lookups on the range because it requires two radix tree tag lookups rather than just one index lookup. i.e. miss the dirty page because it went dirty->writeback during the dirty tag search, and miss the same page when doing the writeback lookup because it went writeback->clean very quickly due to IO completion.

So to stop that from happening, it requires that filesystems can exclude unwritten extent conversion from happening while a SEEK_HOLE/SEEK_DATA operation is in progress, and that the filesystem can safely do mapping tree lookups while providing that extent tree exclusion. I know that XFS has no problems here, but filesystems that use i_mutex for everything might be in trouble.

Quoted 4 lines

Besides, if we just look for pages in the cache over unwritten extents (i.e. someone has treated it as data already), then it can be done locklessly without having to worry about page state changes occurring concurrently...

I'd like to understand why it's important to care about locking here. As I understand it, SEEK_* is used for example to copy a file efficiently. If that is performed on a file that is currently being written to, the resulting copy will probably be bogus anyway, even without SEEK_* usage.

There might be a case where it is important, but I can't see that atm.

It's a data corruption problem, pure and simple....

Quoted 1 line · On Mittwoch, 5. Oktober 2011 Dave Chinner wrote:

It's a data corruption problem, pure and simple....

Thanks for the thorough explanation. So it's a problem when a file has recently been modified and not yet been written back to disk. Would it be worth to force a flush to disk before SEEK_* operations can start? I don't know if it's easier to do all the lookups you suggested, or do an fsync. That could have it's own impacts, though.

Just curious: did you start looking into implementing SEEK_DATA/SEEK_HOLE support?

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

Hi Christoph,

Quoted 2 lines · Christoph Hellwig

Just curious: did you start looking into implementing SEEK_DATA/SEEK_HOLE support?

I have misunderstood your previous response:

"Dave mentioned he had a basic implementation, he might have some code that you can improve on.?

So I am still waiting that now.

But actually I have already wrote a patch at that time, I'll run a few tests and send it out for your guys review soon.

Hello,

Sorry for the delay! I have worked on another high priority task these days.

So the below patch is the first try to add SEEK_DATA/SEEK_HOLE to XFS, I have tested it through seek_test.c from Sunil, looks all works to me.
http://oss.oracle.com/~smushran/seek_data/seek_test.c

Any feedback are welcome, thank you!

Signed-off-by Jie Liu <[email protected]>

---
 fs/xfs/xfs_bmap.c |   81 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_bmap.h |    1 +
 fs/xfs/xfs_file.c |   51 ++++++++++++++++++++++++++++++++-
 fs/xfs/xfs_iops.c |   64 +++++++++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_iops.h |    3 ++
 5 files changed, 199 insertions(+), 1 deletions(-)

fs/xfs/xfs_bmap.c +81 -0

index c68baeb..04c3930 100644
@@ -6133,3 +6133,84 @@ next_block:
 
 	return error;
 }
+
+
+int
+xfs_seek_data_hole(
+	struct xfs_inode	*ip,
+	loff_t			*start,
+	u32			type)
+{
+	xfs_mount_t		*mp = ip->i_mount;
+	xfs_fileoff_t		seekoff = *start;
+	xfs_fileoff_t		filelen;
+	xfs_extnum_t		lastx;
+	xfs_ifork_t		*ifp;
+	struct xfs_bmbt_irec	got;
+	struct xfs_bmbt_irec	prev;
+	u64			extoff;
+	u64			extlen;
+	int			eof;
+
+	if (xfs_get_extsz_hint(ip) ||
+	    ip->i_d.di_flags & (XFS_DIFLAG_PREALLOC | XFS_DIFLAG_APPEND)) {
+		filelen = XFS_MAXIOFFSET(mp);
+	} else {
+		filelen = ip->i_size;
+	}
+
+	ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
+	xfs_bmap_search_extents(ip, XFS_BB_TO_FSBT(mp, BTOBB(*start)),
+				XFS_DATA_FORK, &eof, &lastx, &got, &prev);
+
+	if (type == SEEK_DATA) {
+		extoff = BBTOB(XFS_FSB_TO_BB(mp, got.br_startoff));
+		extlen = BBTOB(XFS_FSB_TO_BB(mp, got.br_blockcount));
+
+		if (eof) {
+			if (seekoff < extoff + extlen)
+				*start = seekoff;
+			else {
+				/*
+				 * There is no more data region past the
+				 * supplied offset.
+				 */
+				return XFS_ERROR(ENXIO);
+			}
+		}
+
+		*start = seekoff < extoff ? extoff : seekoff;
+	} else {
+		for (;;) {
+			extoff = BBTOB(XFS_FSB_TO_BB(mp, got.br_startoff));
+			extlen = BBTOB(XFS_FSB_TO_BB(mp, got.br_blockcount));
+			if (eof) {
+				/*
+				 * There might be a hole at the end of the
+				 * file, adjust to the file size.
+				 */
+				if (seekoff >= extoff) {
+					*start = min_t(xfs_fileoff_t, filelen,
+							(extoff + extlen));
+				}
+				break;
+			}
+
+			/* A hole found */
+			if (seekoff < extoff) {
+				*start = seekoff;
+				break;
+			}
+
+			/* Fetch the next extent */
+			seekoff = extoff + extlen;
+			if (++lastx < ifp->if_bytes / sizeof(xfs_bmbt_rec_t))
+				xfs_bmbt_get_all(xfs_iext_get_ext(ifp, lastx),
+						 &got);
+			else
+				eof = 1;
+		}
+	}
+
+	return 0;
+}

fs/xfs/xfs_bmap.h +1 -0

index 89ee672..964e29b 100644
@@ -196,6 +196,7 @@ int	xfs_bunmapi(struct xfs_trans *tp, struct xfs_inode *ip,
 int	xfs_check_nostate_extents(struct xfs_ifork *ifp, xfs_extnum_t idx,
 		xfs_extnum_t num);
 uint	xfs_default_attroffset(struct xfs_inode *ip);
+int	xfs_seek_data_hole(struct xfs_inode *ip, loff_t *start, u32 type);
 
 #ifdef __KERNEL__
 /* bmap to userspace formatter - copy to user & advance pointer */

fs/xfs/xfs_file.c +50 -1

index 753ed9b..bf5471b 100644
@@ -1141,8 +1141,57 @@ xfs_vm_page_mkwrite(
 	return block_page_mkwrite(vma, vmf, xfs_get_blocks);
 }
 
+STATIC loff_t
+xfs_file_llseek(
+	struct file *file,
+	loff_t offset,
+	int origin)
+{
+	struct inode *inode = file->f_mapping->host;
+	int ret;
+
+	if (origin != SEEK_DATA && origin != SEEK_HOLE)
+		return generic_file_llseek(file, offset, origin);
+
+	mutex_lock(&inode->i_mutex);
+	switch (origin) {
+	case SEEK_DATA:
+	case SEEK_HOLE:
+		if (offset >= i_size_read(inode)) {
+			ret = -ENXIO;
+			goto error;
+		}
+
+		ret = xfs_find_desired_extent(inode, &offset, origin);
+		if (ret)
+			goto error;
+	}
+
+	if (offset < 0 && !(file->f_mode & FMODE_UNSIGNED_OFFSET)) {
+		ret = -EINVAL;
+		goto error;
+	}
+
+	if (offset > inode->i_sb->s_maxbytes) {
+		ret = -EINVAL;
+		goto error;
+	}
+
+	if (offset != file->f_pos) {
+		file->f_pos = offset;
+		file->f_version = 0;
+	}
+
+	mutex_unlock(&inode->i_mutex);
+	return offset;
+
+error:
+	mutex_unlock(&inode->i_mutex);
+	return ret;
+}
+
 const struct file_operations xfs_file_operations = {
-	.llseek		= generic_file_llseek,
+	.llseek		= xfs_file_llseek,
 	.read		= do_sync_read,
 	.write		= do_sync_write,
 	.aio_read	= xfs_file_aio_read,

fs/xfs/xfs_iops.c +64 -0

index 23ce927..482c1ff 100644
@@ -1030,6 +1030,70 @@ xfs_vn_fiemap(
 	return 0;
 }
 
+int
+xfs_find_desired_extent(
+	struct inode		*inode,
+	loff_t			*start,
+	u32			type)
+{
+	xfs_inode_t		*ip = XFS_I(inode);
+	xfs_mount_t		*mp = ip->i_mount;
+	struct xfs_ifork	*ifp;
+	int			lock;
+	int			error;
+
+	if (ip->i_d.di_format != XFS_DINODE_FMT_EXTENTS &&
+	    ip->i_d.di_format != XFS_DINODE_FMT_BTREE &&
+	    ip->i_d.di_format != XFS_DINODE_FMT_LOCAL)
+		return XFS_ERROR(EINVAL);
+
+	xfs_ilock(ip, XFS_IOLOCK_SHARED);
+
+	/*
+	 * Flush the delay alloc blocks. Even after flushing the inode,
+	 * there can still be delalloc blocks on the inode beyond EOF
+	 * due to speculative preallocation. These are not removed until
+	 * the release function is called or the inode is inactivated.
+	 * Hence we cannot assert here that ip->i_delayed_blks == 0.
+	 */
+	if (ip->i_delayed_blks || ip->i_size > ip->i_d.di_size) {
+		error = xfs_flush_pages(ip, 0, -1, 0, FI_REMAPF);
+		if (error)
+			goto out_unlock_iolock;
+	}
+
+	lock = xfs_ilock_map_shared(ip);
+
+	if (XFS_FORCED_SHUTDOWN(mp)) {
+		error = EIO;
+		goto out_unlock_ilock;
+	}
+
+	XFS_STATS_INC(xs_blk_mapr);
+	ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
+
+	ASSERT(ifp->if_ext_max ==
+	      XFS_IFORK_SIZE(ip, XFS_DATA_FORK) / (uint)sizeof(xfs_bmbt_rec_t));
+
+	if (!(ifp->if_flags & XFS_IFEXTENTS)) {
+		error = xfs_iread_extents(NULL, ip, XFS_DATA_FORK);
+		if (error)
+			goto out_unlock_ilock;
+	}
+
+	error = xfs_seek_data_hole(ip, start, type);
+
+out_unlock_ilock:
+	xfs_iunlock_map_shared(ip, lock);
+out_unlock_iolock:
+	xfs_iunlock(ip, XFS_IOLOCK_SHARED);
+
+	if (error)
+		return -error;
+
+	return 0;
+}
+
 static const struct inode_operations xfs_inode_operations = {
 	.get_acl		= xfs_get_acl,
 	.getattr		= xfs_vn_getattr,

fs/xfs/xfs_iops.h +0 -0

index ef41c92..ea47abd 100644

Signed-off-by Jie Liu <[email protected]>

fs/xfs/xfs_file.c +6 -0

+STATIC loff_t
+xfs_file_llseek(
+	struct file *file,
+	loff_t offset,
+	int origin)
+{

fs/xfs/xfs_file.c +4 -0

+		if (offset >= i_size_read(inode)) {
+			ret = -ENXIO;
+			goto error;
+		}

Quoted 2 lines · Jeff Liu

Signed-off-by Jie Liu <[email protected]>

Thanks a lot Jeff. A few comments below:

fs/xfs/xfs_bmap.c next_block: · line 6144

Quoted 7 lines

fs/xfs/xfs_bmap.c +7 -0

+int
+xfs_seek_data_hole(
+	struct xfs_inode	*ip,
+	loff_t			*start,
+	u32			type)
+{
+	xfs_mount_t		*mp = ip->i_mount;

please use

	struct xfs_mount	*mp = ip->i_mount;

for all new code.

fs/xfs/xfs_bmap.c next_block: · line 6160

Quoted 6 lines

fs/xfs/xfs_bmap.c +6 -0

+	if (xfs_get_extsz_hint(ip) ||
+	    ip->i_d.di_flags & (XFS_DIFLAG_PREALLOC | XFS_DIFLAG_APPEND)) {
+		filelen = XFS_MAXIOFFSET(mp);
+	} else {
+		filelen = ip->i_size;
+	}

I don't understand this. XFS_MAXIOFFSET is the maximum possible file size in an XFS filesystem - using it with an extent size hint or the prealloc or appen only flags doesn't make sense to me.

fs/xfs/xfs_bmap.c next_block: · line 6166

Quoted 2 lines

fs/xfs/xfs_bmap.c +1 -0

+	if (type == SEEK_DATA) {

xfs_seek_data_hole shares almost no common code between the SEEK_DATA and SEEK_HOLE cases, which suggests it probably should be two different routines.

Quoted 14 lines

fs/xfs/xfs_file.c +12 -0

+STATIC loff_t
+xfs_file_llseek(
+	struct file *file,
+	loff_t offset,
+	int origin)
+{
+	struct inode *inode = file->f_mapping->host;
+	int ret;
+
+	if (origin != SEEK_DATA && origin != SEEK_HOLE)
+		return generic_file_llseek(file, offset, origin);
+

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

Quoted 4 lines

fs/xfs/xfs_file.c +4 -0

+	mutex_lock(&inode->i_mutex);
+	switch (origin) {
+	case SEEK_DATA:
+	case SEEK_HOLE:

Having the if above and then the switch here seems like and odd style. I'd do either an if, or a switch statement for all possible variants, but not both.

Quoted 5 lines

fs/xfs/xfs_file.c +4 -0

+		if (offset >= i_size_read(inode)) {
+			ret = -ENXIO;
+			goto error;
+		}

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

Quoted 10 lines

fs/xfs/xfs_file.c +10 -0

+
+		ret = xfs_find_desired_extent(inode, &offset, origin);
+		if (ret)
+			goto error;
+	}
+
+	if (offset < 0 && !(file->f_mode & FMODE_UNSIGNED_OFFSET)) {
+		ret = -EINVAL;
+		goto error;
+	}

I don't think this could ever happen on XFS.

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

Quoted 4 lines

fs/xfs/xfs_file.c +4 -0

+	if (offset > inode->i_sb->s_maxbytes) {
+		ret = -EINVAL;
+		goto error;
+	}

This also shouldn't happen if the low-level code does the right thing.

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

Quoted 4 lines

fs/xfs/xfs_file.c +4 -0

+	if (offset != file->f_pos) {
+		file->f_pos = offset;
+		file->f_version = 0;
+	}

XFS never uses f_version, no need to update it.

fs/xfs/xfs_iops.c xfs_vn_fiemap( · line 1037

Quoted 6 lines

fs/xfs/xfs_iops.c +5 -0

+int
+xfs_find_desired_extent(
+	struct inode		*inode,
+	loff_t			*start,
+	u32			type)

I think this would better be merged with the code currenly in xfs_file_llseek. Maybe move all the SEEK_DATA/SEEK_HOLE specific code from there to this function?

Also please move this routine to be next to xfs_file_llseek in xfs_file.c, which also means that it can be marked static.

fs/xfs/xfs_iops.c xfs_vn_fiemap( · line 1040

Quoted 4 lines

fs/xfs/xfs_iops.c +3 -0

+{
+	xfs_inode_t		*ip = XFS_I(inode);
+	xfs_mount_t		*mp = ip->i_mount;

Just as above please use the struct versions for new code.

Quoted 9 lines

fs/xfs/xfs_iops.c +9 -0

+	/*
+	 * Flush the delay alloc blocks. Even after flushing the inode,
+	 * there can still be delalloc blocks on the inode beyond EOF
+	 * due to speculative preallocation. These are not removed until
+	 * the release function is called or the inode is inactivated.
+	 * Hence we cannot assert here that ip->i_delayed_blks == 0.
+	 */
+	if (ip->i_delayed_blks || ip->i_size > ip->i_d.di_size) {
+		error = xfs_flush_pages(ip, 0, -1, 0, FI_REMAPF);

fs/xfs/xfs_iops.c xfs_vn_fiemap( · line 1063

Quoted 3 lines

fs/xfs/xfs_iops.c +3 -0

+		if (error)
+			goto out_unlock_iolock;
+	}

For the final version we should get rid of this flush and instead look for pages having dirty unwritten extents in the pagecache and adjust the result based on it. I'm fine with delaying this until all other issues are sorted out.

Signed-off-by Jie Liu <[email protected]>

fs/xfs/xfs_iops.c +21 -0

+int
+xfs_find_desired_extent(
+	struct inode		*inode,
+	loff_t			*start,
+	u32			type)
+{
+	xfs_inode_t		*ip = XFS_I(inode);
+	xfs_mount_t		*mp = ip->i_mount;
+	struct xfs_ifork	*ifp;
+	int			lock;
+	int			error;
+
+	if (ip->i_d.di_format != XFS_DINODE_FMT_EXTENTS &&
+	    ip->i_d.di_format != XFS_DINODE_FMT_BTREE &&
+	    ip->i_d.di_format != XFS_DINODE_FMT_LOCAL)
+		return XFS_ERROR(EINVAL);
+
+	xfs_ilock(ip, XFS_IOLOCK_SHARED);
+
+	/*
+	 * Flush the delay alloc blocks. Even after flushing the inode,

Quoted 2 lines · Jeff Liu

Signed-off-by Jie Liu <[email protected]>

Christoph has commented on the code-related aspects of the patch, so I won't repeat that. I'll comment on structural/design issues instead.

Firstly, the patch splits the functionality arbitrarily over three different files, and I don't think that is necessary. There really is no reason at all for xfs_bmap.c to know anything aout SEEK_HOLE/SEEK_DATA semantics - that file is for extent manipulation and search functions. SEEK semantics should be entirely encoded into the function that deals with the seeking.

Secondly, xfs_find_desired_extent() and xfs_seek_hole_data() should be one function, and named something like xfs_file_seek_extent().

Finally, don't think using low level extent search functions like xfs_bmap_search_extents() is the right level to implement this functionality (see my comments about SEEK_HOLE/SEEK_DATA semantics in xfs-bmap.c above), especially as we already have functions for looking up holes and extents at given offsets.

That is, to find the next hole at or after after a given offset, we already have xfs_bmap_first_unused(). Indeed, this function already has the exact semantics that SEEK_HOLE requires. Simply put:

	case SEEK_HOLE:
		fsb = XFS_B_TO_FSBT(mp, start_offset);
		error = xfs_bmap_first_unused(NULL, ip, 1, &fsb,
						XFS_DATA_FORK);
		if (error)
			return -error;

		if (fsb <= XFS_B_TO_FSBT(mp, start_offset))
			return start_offset;
		return XFS_FSB_TO_B(mp, fsb);

As to the data extent search, I'd prefer you to use xfs_bmapi_read() rather than xfs_bmap_search_extents() directly. I'd prefer that we return unwritten extents as holes rather than data from the initial implementation, and using the low level xfs_bmap_search_extents() makes that quite complex. However, we already have a function for handling that complexity: xfs_bmapi_read().

That is, xfs_bmapi_read() needs to be passed an array of two maps, one for the current offset, and one for the next extent type. This makes one call sufficient for most transitions. Done in a simple loop it will handle all conditions of hole->unwritten->hole.... until it finds a data extent of EOF.

	start_fsbno = XFS_B_TO_FSBT(mp, start_offset);
	while (1) {
		struct xfs_bmbt_irec	maps[2];
		int			nmaps = 2;

		count_fsb = XFS_B_TO_FSB(mp, XFS_MAXIOFFSET(mp));
		error = xfs_bmapi_read(ip, fsbno, count_fsb,
					&maps, &nmaps, XFS_BMAPI_ENTIRE);

		if (error)
			return -error;
		if (!nmaps) {
			/* no extents at given offset, must be beyond EOF */
			return -ENXIO;
		}

		switch (map[0].br_startblock) {
		case DELAYSTARTBLOCK:
			/* landed in an in-memory data extent */
			return map[0].br_startoff;

		default:
			/* landed in an allocated extent */
			if (map[0].br_state == XFS_EXT_NORM) {
				/* a real data extent */
				return map[0].br_startoff;
			}
			/* Fall through to hole handling for unwritten extents */

		case HOLESTARTBLOCK:
			/*
			 * landed in a hole. If the next extent is a data
			 * extent, then return the start of it, otherwise
			 * we need to move the start offset and map more
			 * blocks.
			 */
			if (map[1].br_startblock == DELAYSTARTBLOCK ||
			   ((map[1].br_startblock != HOLESTARTBLOCK &&
			     map[1].br_state == XFS_EXT_NORM)))
				return map[1].br_startoff;

			start_fsbno = map[1].br_startoff + map[1].br_blockcount;
			break;
		}

		if (XFS_FSB_TO_B(mp, start_fsbno) > ip->i_size) {
			/* Beyond EOF now */
			return -ENXIO;
		}
	}

This can pretty much all be done in fs/xfs/xfs_file.c::xfs_file_seek_extent() because all the functions used by the above code are exported from xfs_bmap.c for external use
- that solves the scattering problem and uses interfaces that we already know work in the intended manner.... ;)

BTW:

Quoted 10 lines

fs/xfs/xfs_file.c +10 -0

+xfs_file_llseek(
+	struct file *file,
+	loff_t offset,
+	int origin)
+{
+	struct inode *inode = file->f_mapping->host;
+	int ret;
+
+	if (origin != SEEK_DATA && origin != SEEK_HOLE)
+		return generic_file_llseek(file, offset, origin);

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

Quoted 3 lines

fs/xfs/xfs_file.c +3 -0

+
+	mutex_lock(&inode->i_mutex);
+	switch (origin) {

We don't need the i_mutex to be held here. We only need to hold the ilock in shared mode for this operation to protect against extent list modifications (like unwritten extent conversion and truncation).

Quoted 22 lines

fs/xfs/xfs_iops.c +21 -0

+int
+xfs_find_desired_extent(
+	struct inode		*inode,
+	loff_t			*start,
+	u32			type)
+{
+	xfs_inode_t		*ip = XFS_I(inode);
+	xfs_mount_t		*mp = ip->i_mount;
+	struct xfs_ifork	*ifp;
+	int			lock;
+	int			error;
+
+	if (ip->i_d.di_format != XFS_DINODE_FMT_EXTENTS &&
+	    ip->i_d.di_format != XFS_DINODE_FMT_BTREE &&
+	    ip->i_d.di_format != XFS_DINODE_FMT_LOCAL)
+		return XFS_ERROR(EINVAL);
+
+	xfs_ilock(ip, XFS_IOLOCK_SHARED);
+
+	/*
+	 * Flush the delay alloc blocks. Even after flushing the inode,

fs/xfs/xfs_iops.c xfs_vn_fiemap( · line 1063

Quoted 10 lines

fs/xfs/xfs_iops.c +10 -0

+	 * there can still be delalloc blocks on the inode beyond EOF
+	 * due to speculative preallocation. These are not removed until
+	 * the release function is called or the inode is inactivated.
+	 * Hence we cannot assert here that ip->i_delayed_blks == 0.
+	 */
+	if (ip->i_delayed_blks || ip->i_size > ip->i_d.di_size) {
+		error = xfs_flush_pages(ip, 0, -1, 0, FI_REMAPF);
+		if (error)
+			goto out_unlock_iolock;
+	}

i.e. this IOLOCK and flush is completely unnecessary because we'll find delayed allocation extents in the extent lookup and can handle them just like real allocated extents....

fs/xfs/xfs_iops.c xfs_vn_fiemap( · line 1065

Quoted 1 line

fs/xfs/xfs_iops.c +1 -0

+	lock = xfs_ilock_map_shared(ip);

i.e. this is the only lock we need to take.

Cheers,

Dave.

Quoted 9 lines

		fsb = XFS_B_TO_FSBT(mp, start_offset);
		error = xfs_bmap_first_unused(NULL, ip, 1, &fsb,
						XFS_DATA_FORK);
		if (error)
			return -error;

		if (fsb <= XFS_B_TO_FSBT(mp, start_offset))
			return start_offset;
		return XFS_FSB_TO_B(mp, fsb);

Thanks for pointing it out, I even don't know XFS has this convenient routine at that time. :(

Quoted 10 lines · Jeff Liu

Quoted 9 lines

		fsb = XFS_B_TO_FSBT(mp, start_offset);
		error = xfs_bmap_first_unused(NULL, ip, 1, &fsb,
						XFS_DATA_FORK);
		if (error)
			return -error;

		if (fsb <= XFS_B_TO_FSBT(mp, start_offset))
			return start_offset;
		return XFS_FSB_TO_B(mp, fsb);

Quoted 1 line

Thanks for pointing it out, I even don't know XFS has this convenient routine at that time. :(

I didn't remember it either, but Dave has been working the dir code which makes use of this funtion lately :)

Btw, the documentation for the function doesn't mention that it starts searching for the hole only after the passed in block number, which is something that could be improved.