On 2018年07月08日 23:51, Christoph Hellwig wrote:
On Fri, Jul 06, 2018 at 11:12:26AM +0800, Shan Hai wrote:
The local format inode is a legal citizen from now on and consider
it in misc operations.
This probably wants to go towards the front of the series, and split
into multiple patches with better changelogs for each.
if (unlikely(XFS_TEST_ERROR(
(XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
- XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE),
+ XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE &&
+ XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL),
This are all the three possible values. It is kinda confusing to read
the checks this way as we alreayd checked for a valid fork type in
the inode read verifies.
Also this seems to be in xfs_bmapi_read, for which reading allowing
inline format forks seems wrong to me.
Agreed.
- if (!(ifp->if_flags & XFS_IFEXTENTS)) {
+ if (!(ifp->if_flags & (XFS_IFINLINE | XFS_IFEXTENTS))) {
error = xfs_iread_extents(NULL, ip, whichfork);
if (error)
return error;
I think we need to clean this up better. We probably want an
xfs_inode_has_extents helper, or even hide the check inside
xfs_iread_extents itself. Also I wonder if the above is read, the
XFS_IFEXTENTS flag has always meant that we have the current extent list
in memory, which should always be the case for a fork in inline format.
Yes, you are right, seems the above 2 checks are left over from my code
cleanup before
sending it out, my apology for wasting your time like this, really sorry.
@@ -5244,11 +5239,18 @@ __xfs_bunmapi(
ifp = XFS_IFORK_PTR(ip, whichfork);
if (unlikely(
XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
+ XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL &&
XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE)) {
XFS_ERROR_REPORT("xfs_bunmapi", XFS_ERRLEVEL_LOW,
ip->i_mount);
return -EFSCORRUPTED;
}
+
+ if (XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_LOCAL) {
+ *rlen = 0;
+ return 0;
+ }
I don't think we should ever call bunmapi for an inline format fork.
};
-
/*
Spurious whitespace change.
* This routine is called to map an inode to the buffer containing the on-disk
* version of the inode. It returns a pointer to the buffer containing the
@@ -384,12 +383,7 @@ xfs_dinode_verify_fork(
switch (XFS_DFORK_FORMAT(dip, whichfork)) {
case XFS_DINODE_FMT_LOCAL:
- /*
- * no local regular files yet
- */
if (whichfork == XFS_DATA_FORK) {
- if (S_ISREG(be16_to_cpu(dip->di_mode)))
- return __this_address;
I think you need to check the new feature bit here and only allow the
inline regular case if the feature bit it set.
OK, I will do that.
@@ -283,7 +283,7 @@ xfs_scrub_dinode(
xfs_scrub_ino_set_corrupt(sc, ino);
break;
case XFS_DINODE_FMT_LOCAL:
- if (!S_ISDIR(mode) && !S_ISLNK(mode))
+ if (!S_ISREG(mode) && !S_ISDIR(mode) && !S_ISLNK(mode))
xfs_scrub_ino_set_corrupt(sc, ino);
break;
Same here.
Ditto.
Thanks
Shan Hai
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
