Skip to contentSkip to conversation

Introduce SEEK_DATA/SEEK_HOLE support V8

PATCH In review Jeff Liu 2 participants 6 messages Feb 17, 2012 – May 10, 2012 XFS filesystem

lore
Share & tools
lore

Jeff Liu

author
#0 lore raw

PATCH 1/1 Introduce SEEK_DATA/SEEK_HOLE support V8

Hello,

This is the revised patch according to Dave's comments for V7.

Changes to V8:
--------------
1. If there is an internal error raised at extent reading routine, just return it rather than ENXIO.
2. Add the commit message.
3. Remove the for(;;) loop since there is no continuous holes shown even if create a Petabyte sparse file with hole extent length longer than 32-bit. Thanks Dave for helping verify that!
4. In xfs_seek_data(), s/len/end/, looks 'end' is more meaningful here to indicate the range of extents mapped.
5. Remove BUG() from xfs_seek_data() since xfs_bmapi_read() have found any corruption during the lookup, it should not occurred at all.

Any comments are appreciated!

Thanks,
-Jeff

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

---
This patch adds lseek(2) SEEK_DATA/SEEK_HOLE functionality.

 fs/xfs/xfs_file.c |  135
++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 134 insertions(+), 1 deletions(-)
fs/xfs/xfs_file.c +134 -1
index 753ed9b..1b3021c 100644
@@ -1141,8 +1141,141 @@ xfs_vm_page_mkwrite(
 	return block_page_mkwrite(vma, vmf, xfs_get_blocks);
 }
 
+STATIC loff_t
+xfs_seek_data(
+	struct file		*file,
+	loff_t			start,
+	u32			type)
+{
+	struct inode		*inode = file->f_mapping->host;
+	struct xfs_inode	*ip = XFS_I(inode);
+	struct xfs_mount	*mp = ip->i_mount;
+	struct xfs_bmbt_irec	map[2];
+	int			nmap = 2;
+	loff_t			uninitialized_var(offset);
+	xfs_fsize_t		isize;
+	xfs_fileoff_t		fsbno;
+	xfs_filblks_t		end;
+	uint			lock;
+	int			error;
+
+	lock = xfs_ilock_map_shared(ip);
+
+	isize = i_size_read(inode);
+	if (start >= isize) {
+		error = ENXIO;
+		goto out_unlock;
+	}
+
+	fsbno = XFS_B_TO_FSBT(mp, start);
+
+	/*
+	 * Try to read extents from the first block indicated
+	 * by fsbno to the end block of the file.
+	 */
+	end = XFS_B_TO_FSB(mp, isize);
+
+	error = xfs_bmapi_read(ip, fsbno, end - fsbno, map, &nmap,
+			       XFS_BMAPI_ENTIRE);
+	if (error)
+		goto out_unlock;
+
+	/*
+	 * Treat unwritten extent as data extent since it might
+	 * contains dirty data in page cache.
+	 */
+	if (map[0].br_startblock != HOLESTARTBLOCK) {
+		offset = max_t(loff_t, start,
+			       XFS_FSB_TO_B(mp, map[0].br_startoff));
+	} else {
+		if (nmap == 1) {
+			error = ENXIO;
+			goto out_unlock;
+		}
+
+		offset = max_t(loff_t, start,
+			       XFS_FSB_TO_B(mp, map[1].br_startoff));
+	}
+
+	if (offset != file->f_pos)
+		file->f_pos = offset;
+
+out_unlock:
+	xfs_iunlock_map_shared(ip, lock);
+
+	if (error)
+		return -error;
+	return offset;
+}
+
+STATIC loff_t
+xfs_seek_hole(
+	struct file		*file,
+	loff_t			start,
+	u32			type)
+{
+	struct inode		*inode = file->f_mapping->host;
+	struct xfs_inode	*ip = XFS_I(inode);
+	struct xfs_mount	*mp = ip->i_mount;
+	loff_t			uninitialized_var(offset);
+	loff_t			holeoff;
+	xfs_fsize_t		isize;
+	xfs_fileoff_t		fsbno;
+	uint			lock;
+	int			error;
+
+	lock = xfs_ilock_map_shared(ip);
+
+	isize = i_size_read(inode);
+	if (start >= isize) {
+		error = ENXIO;
+		goto out_unlock;
+	}
+
+	fsbno = XFS_B_TO_FSBT(mp, start);
+	error = xfs_bmap_first_unused(NULL, ip, 1, &fsbno, XFS_DATA_FORK);
+	if (error)
+		goto out_unlock;
+
+	holeoff = XFS_FSB_TO_B(mp, fsbno);
+	if (holeoff <= start)
+		offset = start;
+	else
+		offset = min_t(loff_t, holeoff, isize);
+
+	if (offset != file->f_pos)
+		file->f_pos = offset;
+
+out_unlock:
+	xfs_iunlock_map_shared(ip, lock);
+
+	if (error)
+		return -error;
+	return offset;
+}
+
+STATIC loff_t
+xfs_file_llseek(
+	struct file	*file,
+	loff_t		offset,
+	int		origin)
+{
+	switch (origin) {
+	case SEEK_END:
+	case SEEK_CUR:
+	case SEEK_SET:
+		return generic_file_llseek(file, offset, origin);
+	case SEEK_DATA:
+		return xfs_seek_data(file, offset, origin);
+	case SEEK_HOLE:
+		return xfs_seek_hole(file, offset, origin);
+	default:
+		return -EINVAL;
+	}
+}
+
 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,
Signature · 6 lines
1.7.9

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Reviewed-by on v1 1/1
#1 lore raw
Quoted 24 lines · Jeff Liu

Hello,

This is the revised patch according to Dave's comments for V7.

Changes to V8:
--------------
1. If there is an internal error raised at extent reading routine, just return it rather than ENXIO.
2. Add the commit message.
3. Remove the for(;;) loop since there is no continuous holes shown even if create a Petabyte sparse file with hole extent length longer than 32-bit. Thanks Dave for helping verify that!
4. In xfs_seek_data(), s/len/end/, looks 'end' is more meaningful here to indicate the range of extents mapped.
5. Remove BUG() from xfs_seek_data() since xfs_bmapi_read() have found any corruption during the lookup, it should not occurred at all.

Any comments are appreciated!

Thanks,
-Jeff

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

...

Quoted 2 lines
fs/xfs/xfs_file.c +2 -0
+STATIC loff_t
+xfs_seek_hole(

...

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

Quoted 12 lines
fs/xfs/xfs_file.c +12 -0
+
+	fsbno = XFS_B_TO_FSBT(mp, start);
+	error = xfs_bmap_first_unused(NULL, ip, 1,&fsbno, XFS_DATA_FORK);
+	if (error)
+		goto out_unlock;
+
+	holeoff = XFS_FSB_TO_B(mp, fsbno);
+	if (holeoff<= start)
+		offset = start;
+	else
+		offset = min_t(loff_t, holeoff, isize);
+

...

Very Nice. Much more concise.

Can xfs_bmap_first_unused() return something larger than the end of file?

Reviewed-by Mark Tinguely <[email protected]>

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

author
#2 lore raw
Quoted 46 lines · Mark Tinguely
Quoted 24 lines · Jeff Liu

Hello,

This is the revised patch according to Dave's comments for V7.

Changes to V8:
--------------
1. If there is an internal error raised at extent reading routine, just return it rather than ENXIO.
2. Add the commit message.
3. Remove the for(;;) loop since there is no continuous holes shown even if create a Petabyte sparse file with hole extent length longer than 32-bit. Thanks Dave for helping verify that!
4. In xfs_seek_data(), s/len/end/, looks 'end' is more meaningful here to indicate the range of extents mapped.
5. Remove BUG() from xfs_seek_data() since xfs_bmapi_read() have found any corruption during the lookup, it should not occurred at all.

Any comments are appreciated!

Thanks,
-Jeff

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

...

Quoted 2 lines
fs/xfs/xfs_file.c +2 -0
+STATIC loff_t
+xfs_seek_hole(

...

Quoted 12 lines
fs/xfs/xfs_file.c +12 -0
+
+    fsbno = XFS_B_TO_FSBT(mp, start);
+    error = xfs_bmap_first_unused(NULL, ip, 1,&fsbno, XFS_DATA_FORK);
+    if (error)
+        goto out_unlock;
+
+    holeoff = XFS_FSB_TO_B(mp, fsbno);
+    if (holeoff<= start)
+        offset = start;
+    else
+        offset = min_t(loff_t, holeoff, isize);
+

...

Quoted 3 lines

Very Nice. Much more concise.

Can xfs_bmap_first_unused() return something larger than the end of file?

I think it could be happen if the file has no holes past the given offset. In this case, it will return the first block past the end of file. That is why "min_t()" is used to determine the final value.

Thanks,
-Jeff

Quoted 2 lines
Reviewed-by Mark Tinguely <[email protected]>

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

#3 lore raw
Quoted 50 lines · Jeff Liu
Quoted 48 lines · Mark Tinguely
Quoted 24 lines · Jeff Liu

Hello,

This is the revised patch according to Dave's comments for V7.

Changes to V8:
--------------
1. If there is an internal error raised at extent reading routine, just return it rather than ENXIO.
2. Add the commit message.
3. Remove the for(;;) loop since there is no continuous holes shown even if create a Petabyte sparse file with hole extent length longer than 32-bit. Thanks Dave for helping verify that!
4. In xfs_seek_data(), s/len/end/, looks 'end' is more meaningful here to indicate the range of extents mapped.
5. Remove BUG() from xfs_seek_data() since xfs_bmapi_read() have found any corruption during the lookup, it should not occurred at all.

Any comments are appreciated!

Thanks,
-Jeff

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

...

Quoted 2 lines
fs/xfs/xfs_file.c +2 -0
+STATIC loff_t
+xfs_seek_hole(

...

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

Quoted 12 lines
fs/xfs/xfs_file.c +12 -0
+
+    fsbno = XFS_B_TO_FSBT(mp, start);
+    error = xfs_bmap_first_unused(NULL, ip, 1,&fsbno, XFS_DATA_FORK);
+    if (error)
+        goto out_unlock;
+
+    holeoff = XFS_FSB_TO_B(mp, fsbno);
+    if (holeoff<= start)
+        offset = start;
+    else
+        offset = min_t(loff_t, holeoff, isize);
+

...

Very Nice. Much more concise.

Quoted 13 lines
Quoted 1 line

Can xfs_bmap_first_unused() return something larger than the end of file?

I think it could be happen if the file has no holes past the given offset. In this case, it will return the first block past the end of file. That is why "min_t()" is used to determine the final value.

Thanks,
-Jeff

Quoted 2 lines
Reviewed-by Mark Tinguely<[email protected]>

Okay. Looks good.

--Mark Tinguely.

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

#4 lore raw
In reply to Jeff Liu
Quoted 62 lines · Jeff Liu

Hello,

This is the revised patch according to Dave's comments for V7.

Changes to V8:
--------------
1. If there is an internal error raised at extent reading routine, just return it rather than ENXIO.
2. Add the commit message.
3. Remove the for(;;) loop since there is no continuous holes shown even if create a Petabyte sparse file with hole extent length longer than 32-bit. Thanks Dave for helping verify that!
4. In xfs_seek_data(), s/len/end/, looks 'end' is more meaningful here to indicate the range of extents mapped.
5. Remove BUG() from xfs_seek_data() since xfs_bmapi_read() have found any corruption during the lookup, it should not occurred at all.

Any comments are appreciated!

Thanks,
-Jeff

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

---
This patch adds lseek(2) SEEK_DATA/SEEK_HOLE functionality.

fs/xfs/xfs_file.c +35 -0
+STATIC loff_t
+xfs_seek_hole(
+	struct file		*file,
+	loff_t			start,
+	u32			type)
+{
+	struct inode		*inode = file->f_mapping->host;
+	struct xfs_inode	*ip = XFS_I(inode);
+	struct xfs_mount	*mp = ip->i_mount;
+	loff_t			uninitialized_var(offset);
+	loff_t			holeoff;
+	xfs_fsize_t		isize;
+	xfs_fileoff_t		fsbno;
+	uint			lock;
+	int			error;
+
+	lock = xfs_ilock_map_shared(ip);
+
+	isize = i_size_read(inode);
+	if (start>= isize) {
+		error = ENXIO;
+		goto out_unlock;
+	}
+
+	fsbno = XFS_B_TO_FSBT(mp, start);
+	error = xfs_bmap_first_unused(NULL, ip, 1,&fsbno, XFS_DATA_FORK);
+	if (error)
+		goto out_unlock;
+
+	holeoff = XFS_FSB_TO_B(mp, fsbno);
+	if (holeoff<= start)
+		offset = start;
+	else
+		offset = min_t(loff_t, holeoff, isize);
+

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

Quoted 10 lines
fs/xfs/xfs_file.c +10 -0
+	if (offset != file->f_pos)
+		file->f_pos = offset;
+
+out_unlock:
+	xfs_iunlock_map_shared(ip, lock);
+
+	if (error)
+		return -error;
+	return offset;
+}

Sorry this got set aside. This is being dusted off for inclusion.

Refresh my memory, do we need a XFS_FORCED_SHUTDOWN test in xfs_seek_hole()? In the earlier versions it was covered in the xfs_bmapi_read().

A comment on the min_t() that the xfs_bmap_first_unused() could return a value bigger than the isize if there are no more holes may help the forgetful such as myself.

Otherwise it looks good.

Thank-you.

Mark Tinguely <[email protected]>

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

author
#5 lore raw
Quoted 67 lines · Mark Tinguely
Quoted 66 lines · Jeff Liu

Hello,

This is the revised patch according to Dave's comments for V7.

Changes to V8:
--------------
1. If there is an internal error raised at extent reading routine, just return it rather than ENXIO.
2. Add the commit message.
3. Remove the for(;;) loop since there is no continuous holes shown even if create a Petabyte sparse file with hole extent length longer than 32-bit. Thanks Dave for helping verify that!
4. In xfs_seek_data(), s/len/end/, looks 'end' is more meaningful here to indicate the range of extents mapped.
5. Remove BUG() from xfs_seek_data() since xfs_bmapi_read() have found any corruption during the lookup, it should not occurred at all.

Any comments are appreciated!

Thanks,
-Jeff

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

---
This patch adds lseek(2) SEEK_DATA/SEEK_HOLE functionality.

fs/xfs/xfs_file.c +39 -0
+STATIC loff_t
+xfs_seek_hole(
+    struct file        *file,
+    loff_t            start,
+    u32            type)
+{
+    struct inode        *inode = file->f_mapping->host;
+    struct xfs_inode    *ip = XFS_I(inode);
+    struct xfs_mount    *mp = ip->i_mount;
+    loff_t            uninitialized_var(offset);
+    loff_t            holeoff;
+    xfs_fsize_t        isize;
+    xfs_fileoff_t        fsbno;
+    uint            lock;
+    int            error;
+
+    lock = xfs_ilock_map_shared(ip);
+
+    isize = i_size_read(inode);
+    if (start>= isize) {
+        error = ENXIO;
+        goto out_unlock;
+    }
+
+    fsbno = XFS_B_TO_FSBT(mp, start);
+    error = xfs_bmap_first_unused(NULL, ip, 1,&fsbno, XFS_DATA_FORK);
+    if (error)
+        goto out_unlock;
+
+    holeoff = XFS_FSB_TO_B(mp, fsbno);
+    if (holeoff<= start)
+        offset = start;
+    else
+        offset = min_t(loff_t, holeoff, isize);
+
+    if (offset != file->f_pos)
+        file->f_pos = offset;
+
+out_unlock:
Quoted 8 lines

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

Quoted 6 lines
fs/xfs/xfs_file.c +6 -0
+    xfs_iunlock_map_shared(ip, lock);
+
+    if (error)
+        return -error;
+    return offset;
+}

Sorry this got set aside. This is being dusted off for inclusion.

Thanks for picking this up!

Quoted 4 lines

Refresh my memory, do we need a XFS_FORCED_SHUTDOWN test in xfs_seek_hole()? In the earlier versions it was covered in the xfs_bmapi_read().

Yes, definitely.

Quoted 4 lines

A comment on the min_t() that the xfs_bmap_first_unused() could return a value bigger than the isize if there are no more holes may help the forgetful such as myself.

I'll leave a comment for it, it would be helpful to recall the case for a long period.

Thanks,
-Jeff

Quoted 11 lines

Otherwise it looks good.

Thank-you.

Mark Tinguely <[email protected]>

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

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