Enable per-file/per-directory DAX operations V4

From: Ira Weiny <[email protected]>

xfs_reinit_inode() -> inode_init_always() already handles calling init_rwsem(i_rwsem). Doing so again is unneeded.

Signed-off-by Ira Weiny <[email protected]>

Quoted 6 lines · On Thu, Feb 20, 2020 at 04:41:22PM -0800, [email protected] wrote:

From: Ira Weiny <[email protected]>

xfs_reinit_inode() -> inode_init_always() already handles calling init_rwsem(i_rwsem). Doing so again is unneeded.

Signed-off-by Ira Weiny <[email protected]>

Except that this inode has been destroyed and freed by the VFS, and we are now recycling it back into the VFS before we actually physically freed it.

Hence we have re-initialise the semaphore because the semaphore can contain internal state that is specific to it's new life cycle (e.g. the lockdep context) that will cause problems if we just assume that the inode is the same inode as it was before we recycled it back into the VFS caches.

So, yes, we actually do need to re-initialise the rwsem here.

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

From: Ira Weiny <[email protected]>

xfs_isilocked() can't work fully without CONFIG_LOCKDEP. However, making xfs_isilocked() dependant on CONFIG_LOCKDEP is not feasible because it is used for more than the i_rwsem. Therefore a short-circuit was provided via debug_locks. However, this caused confusion while working through the xfs locking.

Rather than use debug_locks as a flag specify this clearly using IS_ENABLED(CONFIG_LOCKDEP).

Signed-off-by Ira Weiny <[email protected]>

---
Changes from V3:
Reordered to be a "pre-cleanup" patch

Changes from V2:
This patch is new for V3

---
 fs/xfs/xfs_inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/xfs/xfs_inode.c +0 -0

index c5077e6326c7..35df324875db 100644

Quoted 28 lines · On Thu, Feb 20, 2020 at 04:41:23PM -0800, [email protected] wrote:

From: Ira Weiny <[email protected]>

xfs_isilocked() can't work fully without CONFIG_LOCKDEP. However, making xfs_isilocked() dependant on CONFIG_LOCKDEP is not feasible because it is used for more than the i_rwsem. Therefore a short-circuit was provided via debug_locks. However, this caused confusion while working through the xfs locking.

Rather than use debug_locks as a flag specify this clearly using IS_ENABLED(CONFIG_LOCKDEP).

Signed-off-by Ira Weiny <[email protected]>

---
Changes from V3:
Reordered to be a "pre-cleanup" patch

Changes from V2:
This patch is new for V3

---
 fs/xfs/xfs_inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/xfs/xfs_inode.c +0 -0

index c5077e6326c7..35df324875db 100644

fs/xfs/xfs_inode.c xfs_isilocked( · line 368

Quoted 6 lines

fs/xfs/xfs_inode.c +1 -1

@@ -364,7 +364,7 @@ xfs_isilocked(
 
 	if (lock_flags & (XFS_IOLOCK_EXCL|XFS_IOLOCK_SHARED)) {
 		if (!(lock_flags & XFS_IOLOCK_SHARED))
-			return !debug_locks ||
+			return !IS_ENABLED(CONFIG_LOCKDEP) ||
 				lockdep_is_held_type(&VFS_I(ip)->i_rwsem, 0);

This breaks expected lockdep behaviour.

We need to use debug_locks here because lockdep turns off lock checking via debug_locks when lockdep encounters a locking inconsistency. We only want to know about the first locking problem, not spew cascading lock problems over and over once we already know there is a locking problem.

IOWs, checking debug_locks is required here for the same reason it is used in lockdep_assert_held_{read/write}(). essentially we are open coding lockdep_assert_held_write() here because this function is only called from within ASSERT() statements and we don't want multiple WARN/BUGs being issued when this triggers....

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

From: Ira Weiny <[email protected]>

Remove the check because DAX now has it's own read/write methods and file systems which support DAX check IS_DAX() prior to IOCB_DIRECT on their own. Therefore, it does not matter if the file state is DAX when the iocb flags are created.

Reviewed-by Jan Kara <[email protected]>

Signed-off-by Ira Weiny <[email protected]>

Quoted 7 lines · On Thu, Feb 20, 2020 at 04:41:24PM -0800, [email protected] wrote:

From: Ira Weiny <[email protected]>

Remove the check because DAX now has it's own read/write methods and file systems which support DAX check IS_DAX() prior to IOCB_DIRECT on their own. Therefore, it does not matter if the file state is DAX when the iocb flags are created.

Quoted 2 lines

Reviewed-by Jan Kara <[email protected]>

Signed-off-by Ira Weiny <[email protected]>

Yup, looks good.

Reviewed-by Dave Chinner <[email protected]>

Signature · 2 lines

Dave Chinner
[email protected]

From: Ira Weiny <[email protected]>

Remove the check because DAX now has it's own read/write methods and file systems which support DAX check IS_DAX() prior to IOCB_DIRECT on their own. Therefore, it does not matter if the file state is DAX when the iocb flags are created.

Reviewed-by Jan Kara <[email protected]>

Signed-off-by Ira Weiny <[email protected]>

---
Changes from v3:
Reword commit message.
Reordered to be a 'pre-cleanup' patch

---
 include/linux/fs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

include/linux/fs.h +0 -0

index 3cd4fe6b845e..63d1e533a07d 100644

From: Ira Weiny <[email protected]>

xfs_inode_supports_dax() should reflect if the inode can support DAX not that it is enabled for DAX.

Change the use of xfs_inode_supports_dax() to reflect only if the inode and underlying storage support dax.

Add a new function xfs_inode_enable_dax() which reflects if the inode

should be enabled for DAX.

Signed-off-by Ira Weiny <[email protected]>

---
Changes from v3:
Update functions and names to be more clear Update commit message
Merge with
'fs/xfs: Clean up DAX support check' don't allow IS_DAX() on a directory use STATIC macro for static make xfs_inode_supports_dax() static

---
 fs/xfs/xfs_iops.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

fs/xfs/xfs_iops.c +19 -6

index 81f2f93caec0..ff711efc5247 100644
@@ -1237,19 +1237,18 @@ static const struct inode_operations xfs_inline_symlink_inode_operations = {
 };
 
 /* Figure out if this file actually supports DAX. */
-static bool
+STATIC bool
 xfs_inode_supports_dax(
 	struct xfs_inode	*ip)
 {
 	struct xfs_mount	*mp = ip->i_mount;
 
 	/* Only supported on non-reflinked files. */
-	if (!S_ISREG(VFS_I(ip)->i_mode) || xfs_is_reflink_inode(ip))
+	if (xfs_is_reflink_inode(ip))
 		return false;
 
-	/* DAX mount option or DAX iflag must be set. */
-	if (!(mp->m_flags & XFS_MOUNT_DAX) &&
-	    !(ip->i_d.di_flags2 & XFS_DIFLAG2_DAX))
+	/* Only supported on regular files. */
+	if (!S_ISREG(VFS_I(ip)->i_mode))
 		return false;
 
 	/* Block size must match page size */

@@ -1260,6 +1259,20 @@ xfs_inode_supports_dax(
 	return xfs_inode_buftarg(ip)->bt_daxdev != NULL;
 }
 
+STATIC bool
+xfs_inode_enable_dax(
+	struct xfs_inode *ip)
+{
+	if (!xfs_inode_supports_dax(ip))
+		return false;
+
+	if (ip->i_d.di_flags2 & XFS_DIFLAG2_DAX)
+		return true;
+	if (ip->i_mount->m_flags & XFS_MOUNT_DAX)
+		return true;
+	return false;
+}
+
 STATIC void
 xfs_diflags_to_iflags(
 	struct inode		*inode,

@@ -1278,7 +1291,7 @@ xfs_diflags_to_iflags(
 		inode->i_flags |= S_SYNC;
 	if (flags & XFS_DIFLAG_NOATIME)
 		inode->i_flags |= S_NOATIME;
-	if (xfs_inode_supports_dax(ip))
+	if (xfs_inode_enable_dax(ip))
 		inode->i_flags |= S_DAX;
 }
 

Signature · 1 line

2.21.0

Quoted 5 lines · On Thu, Feb 20, 2020 at 04:41:27PM -0800, [email protected] wrote:

From: Ira Weiny <[email protected]>

xfs_inode_supports_dax() should reflect if the inode can support DAX not that it is enabled for DAX.

Quoted 4 lines

Change the use of xfs_inode_supports_dax() to reflect only if the inode and underlying storage support dax.

Add a new function xfs_inode_enable_dax() which reflects if the inode

Heavily into bikeshedding here, but "enable" sounds like a verb, but this function doesn't actually turn dax on for a file, it merely decides if we /should/ turn it on.

xfs_inode_wants_dax() ?

--D

Quoted 78 lines

should be enabled for DAX.

Signed-off-by Ira Weiny <[email protected]>

---
Changes from v3:
Update functions and names to be more clear Update commit message
Merge with
'fs/xfs: Clean up DAX support check' don't allow IS_DAX() on a directory use STATIC macro for static make xfs_inode_supports_dax() static

---
 fs/xfs/xfs_iops.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

fs/xfs/xfs_iops.c +19 -6

index 81f2f93caec0..ff711efc5247 100644
@@ -1237,19 +1237,18 @@ static const struct inode_operations xfs_inline_symlink_inode_operations = {
 };
 
 /* Figure out if this file actually supports DAX. */
-static bool
+STATIC bool
 xfs_inode_supports_dax(
 	struct xfs_inode	*ip)
 {
 	struct xfs_mount	*mp = ip->i_mount;
 
 	/* Only supported on non-reflinked files. */
-	if (!S_ISREG(VFS_I(ip)->i_mode) || xfs_is_reflink_inode(ip))
+	if (xfs_is_reflink_inode(ip))
 		return false;
 
-	/* DAX mount option or DAX iflag must be set. */
-	if (!(mp->m_flags & XFS_MOUNT_DAX) &&
-	    !(ip->i_d.di_flags2 & XFS_DIFLAG2_DAX))
+	/* Only supported on regular files. */
+	if (!S_ISREG(VFS_I(ip)->i_mode))
 		return false;
 
 	/* Block size must match page size */

@@ -1260,6 +1259,20 @@ xfs_inode_supports_dax(
 	return xfs_inode_buftarg(ip)->bt_daxdev != NULL;
 }
 
+STATIC bool
+xfs_inode_enable_dax(
+	struct xfs_inode *ip)
+{
+	if (!xfs_inode_supports_dax(ip))
+		return false;
+
+	if (ip->i_d.di_flags2 & XFS_DIFLAG2_DAX)
+		return true;
+	if (ip->i_mount->m_flags & XFS_MOUNT_DAX)
+		return true;
+	return false;
+}
+
 STATIC void
 xfs_diflags_to_iflags(
 	struct inode		*inode,

@@ -1278,7 +1291,7 @@ xfs_diflags_to_iflags(
 		inode->i_flags |= S_SYNC;
 	if (flags & XFS_DIFLAG_NOATIME)
 		inode->i_flags |= S_NOATIME;
-	if (xfs_inode_supports_dax(ip))
+	if (xfs_inode_enable_dax(ip))
 		inode->i_flags |= S_DAX;
 }
 

Signature · 1 line

2.21.0

From: Ira Weiny <[email protected]>

DAX requires special address space operations (aops). Changing DAX state therefore requires changing those aops.

However, many functions require aops to remain consistent through a deep call stack.

Define a vfs level inode rwsem to protect aops throughout call stacks which require them.

Finally, define calls to be used in subsequent patches when aops usage needs to be quiesced by the file system.

Quoted 5 lines · On Thu, Feb 20, 2020 at 04:41:28PM -0800, [email protected] wrote:

From: Ira Weiny <[email protected]>

DAX requires special address space operations (aops). Changing DAX state therefore requires changing those aops.

Quoted 8 lines

However, many functions require aops to remain consistent through a deep call stack.

Define a vfs level inode rwsem to protect aops throughout call stacks which require them.

Finally, define calls to be used in subsequent patches when aops usage needs to be quiesced by the file system.

I am very much against this. There is a reason why we don't support changes of ops vectors at runtime anywhere else, because it is horribly complicated and impossible to get right. IFF we ever want to change the DAX vs non-DAX mode (which I'm still not sold on) the right way is to just add a few simple conditionals and merge the aops, which is much easier to reason about, and less costly in overall overhead.

Quoted 15 lines · Christoph Hellwig

Quoted 5 lines · On Thu, Feb 20, 2020 at 04:41:28PM -0800, [email protected] wrote:

From: Ira Weiny <[email protected]>

DAX requires special address space operations (aops). Changing DAX state therefore requires changing those aops.

Quoted 8 lines

However, many functions require aops to remain consistent through a deep call stack.

Define a vfs level inode rwsem to protect aops throughout call stacks which require them.

Finally, define calls to be used in subsequent patches when aops usage needs to be quiesced by the file system.

Quoted 7 lines

I am very much against this. There is a reason why we don't support changes of ops vectors at runtime anywhere else, because it is horribly complicated and impossible to get right. IFF we ever want to change the DAX vs non-DAX mode (which I'm still not sold on) the right way is to just add a few simple conditionals and merge the aops, which is much easier to reason about, and less costly in overall overhead.

*cough*

That's exactly what the original code did. And it was broken because page faults call multiple aops that are dependent on the result of the previous aop calls setting up the state correctly for the latter calls. And when S_DAX changes between those calls, shit breaks.

It's exactly the same problem as switching aops between two dependent aops method calls - we don't solve anything by merging aops and checking IS_DAX in each method because the race condition is still there.

/me throws his hands in the air and walks away

Quoted 7 lines

I am very much against this. There is a reason why we don't support changes of ops vectors at runtime anywhere else, because it is horribly complicated and impossible to get right. IFF we ever want to change the DAX vs non-DAX mode (which I'm still not sold on) the right way is to just add a few simple conditionals and merge the aops, which is much easier to reason about, and less costly in overall overhead.

*cough*

That's exactly what the original code did. And it was broken because page faults call multiple aops that are dependent on the result of the previous aop calls setting up the state correctly for the latter calls. And when S_DAX changes between those calls, shit breaks.

Quoted 8 lines · Dave Chinner

Quoted 7 lines

I am very much against this. There is a reason why we don't support changes of ops vectors at runtime anywhere else, because it is horribly complicated and impossible to get right. IFF we ever want to change the DAX vs non-DAX mode (which I'm still not sold on) the right way is to just add a few simple conditionals and merge the aops, which is much easier to reason about, and less costly in overall overhead.

Quoted 7 lines

*cough*

That's exactly what the original code did. And it was broken because page faults call multiple aops that are dependent on the result of the previous aop calls setting up the state correctly for the latter calls. And when S_DAX changes between those calls, shit breaks.

No, the original code was broken because it didn't serialize between DAX and buffer access.

Take a step back and look where the problems are, and they are not mostly with the aops. In fact the only aop useful for DAX is is ->writepages, and how it uses ->writepages is actually a bit of an abuse of that interface.

So what we really need it just a way to prevent switching the flag when a file is mapped,

and in the normal read/write path ensure the
flag can't be switch while I/O is going on, which could either be done by ensuring it is only switched under i_rwsem or equivalent protection, or by setting the DAX flag once in the iocb similar to IOCB_DIRECT.

And they easiest way to get all this done is as a first step to just allowing switching the flag when no blocks are allocated at all, similar to how the rt flag works.

Quoted 7 lines

No, the original code was broken because it didn't serialize between DAX and buffer access.

Take a step back and look where the problems are, and they are not mostly with the aops. In fact the only aop useful for DAX is is ->writepages, and how it uses ->writepages is actually a bit of an abuse of that interface.

The races are all through the fops, too, which is one of the reasons Darrick mentioned we should probably move this up to file ops level...

Quoted 2 lines

So what we really need it just a way to prevent switching the flag when a file is mapped,

That's not sufficient.

We also have to prevent the file from being mapped *while we are switching*. Nothing in the mmap() path actually serialises against filesystem operations, and the initial behavioural checks in the page fault path are similarly unserialised against changing the S_DAX flag.

Quoted 5 lines

and in the normal read/write path ensure the
flag can't be switch while I/O is going on, which could either be done by ensuring it is only switched under i_rwsem or equivalent protection, or by setting the DAX flag once in the iocb similar to IOCB_DIRECT.

The iocb path is not the problem - that's entirely serialised against S_DAX changes by the i_rwsem. The problem is that we have no equivalent filesystem level serialisation for the entire mmap/page fault path, and it checks S_DAX all over the place.

/me wonders if the best thing to do is to add a ->fault callout to tell the filesystem to lock/unlock the inode right up at the top of the page fault path, outside even the mmap_sem. That means all the methods that the page fault calls are protected against S_DAX changes, and it gives us a low cost method of serialising page faults against DIO (e.g. via inode_dio_wait())....

Quoted 4 lines

And they easiest way to get all this done is as a first step to just allowing switching the flag when no blocks are allocated at all, similar to how the rt flag works.

False equivalence - it is not similar because the RT flag changes and their associated state checks are *already fully serialised* by the XFS_ILOCK_EXCL. S_DAX accesses have no such serialisation, and that's the problem we need to solve...

And my point is that if we ensure S_DAX can only be checked if there are no blocks on the file, is is fairly easy to provide the same guarantee. And I've not heard any argument that we really need more flexibility than that. In fact I think just being able to change it on the parent directory and inheriting the flag might be more than plenty, which would lead to a very simple implementation without any of the crazy overhead in this series.

Quoted 7 lines · Christoph Hellwig

And my point is that if we ensure S_DAX can only be checked if there are no blocks on the file, is is fairly easy to provide the same guarantee. And I've not heard any argument that we really need more flexibility than that. In fact I think just being able to change it on the parent directory and inheriting the flag might be more than plenty, which would lead to a very simple implementation without any of the crazy overhead in this series.

I know of one user who had at least mentioned it to me, so I cc'd him. Jonathan, can you describe your use case for being able to change a file between dax and non-dax modes? Or, if I'm misremembering, just correct me?

Thanks!
Jeff

I'm a middleware developer, focused on how Java (JVM) workloads can benefit from app-direct mode pmem. Initially the target is apps that need a fast binary log for fault tolerance: the classic database WAL use case; transaction coordination systems; enterprise message bus persistence and suchlike. Critically, there are cases where we use log based storage, i.e. it's not the strict 'read rarely, only on recovery' model that a classic db may have, but more of a 'append only, read many times' event stream model.

Think of the log oriented data storage as having logical segments (let's implement them as files), of which the most recent is being appended to (read_write) and the remaining N-1 older segments are full and sealed, so effectively immutable (read_only) until discarded. The tail segment needs to be in DAX mode for optimal write performance, as the size of the append may be sub-block and we don't want the overhead of the kernel call anyhow. So that's clearly a good fit for putting on a DAX fs mount and using mmap with MAP_SYNC.

However, we want fast read access into the segments, to retrieve stored records. The small access index can be built in volatile RAM (assuming we're willing to take the startup overhead of a full file scan at recovery time) but the data itself is big and we don't want to move it all off pmem. Which means the requirements are now different: we want the O/S cache to pull hot data into fast volatile RAM for us, which DAX explicitly won't do. Effectively a poor man's 'memory mode' pmem, rather than app-direct mode, except here we're using the O/S rather than the hardware memory controller to do the cache management for us.

Currently this requires closing the full (read_write) file, then copying it to a non-DAX device and reopening it (read_only) there. Clearly that's expensive and rather tedious. Instead, I'd like to close the MAP_SYNC mmap, then, leaving the file where it is, reopen it in a mode that will instead go via the O/S cache in the traditional manner. Bonus points if I can do it over non-overlapping ranges in a file without closing the DAX mode mmap, since then the segments are entirely logical instead of needing separate physical files.

I note a comment below regarding a per-directly setting, but don't have the background to fully understand what's being suggested. However, I'll note here that I can live with a per-directory granularity, as relinking a file into a new dir is a constant time operation, whilst the move described above isn't. So if a per-directory granularity is easier than a per-file one that's fine, though as a person with only passing knowledge of filesystem design I don't see how having multiple links to a file can work cleanly in that case.

P.S. I'll cheekily take the opportunity of having your attention to tack on one minor gripe about the current system: The only way to know if a mmap with MAP_SYNC will work is to try it and catch the error. Which would be reasonable if it were free of side effects. However, the process requires first expanding the file to at least the size of the desired map, which is

done non-atomically i.e. is user visible. There are thus nasty race conditions in the cleanup, where after a failed mmap attempt (e.g the device doesn't support DAX), we try to shrink the file back to its original size, but something else has already opened it at its new, larger size. This is not theoretical: I got caught by it whilst adapting some of our middleware to use pmem. Therefore, some way to probe the file path for its capability would be nice, much the same as I can e.g. inspect file permissions to (more or less) evaluate if I can write it without actually mutating it. Thanks!

Hello,

Quoted 36 lines · Jonathan Halliday

I'm a middleware developer, focused on how Java (JVM) workloads can benefit from app-direct mode pmem. Initially the target is apps that need a fast binary log for fault tolerance: the classic database WAL use case; transaction coordination systems; enterprise message bus persistence and suchlike. Critically, there are cases where we use log based storage, i.e. it's not the strict 'read rarely, only on recovery' model that a classic db may have, but more of a 'append only, read many times' event stream model.

Think of the log oriented data storage as having logical segments (let's implement them as files), of which the most recent is being appended to (read_write) and the remaining N-1 older segments are full and sealed, so effectively immutable (read_only) until discarded. The tail segment needs to be in DAX mode for optimal write performance, as the size of the append may be sub-block and we don't want the overhead of the kernel call anyhow. So that's clearly a good fit for putting on a DAX fs mount and using mmap with MAP_SYNC.

However, we want fast read access into the segments, to retrieve stored records. The small access index can be built in volatile RAM (assuming we're willing to take the startup overhead of a full file scan at recovery time) but the data itself is big and we don't want to move it all off pmem. Which means the requirements are now different: we want the O/S cache to pull hot data into fast volatile RAM for us, which DAX explicitly won't do. Effectively a poor man's 'memory mode' pmem, rather than app-direct mode, except here we're using the O/S rather than the hardware memory controller to do the cache management for us.

Currently this requires closing the full (read_write) file, then copying it to a non-DAX device and reopening it (read_only) there. Clearly that's expensive and rather tedious. Instead, I'd like to close the MAP_SYNC mmap, then, leaving the file where it is, reopen it in a mode that will instead go via the O/S cache in the traditional manner. Bonus points if I can do it over non-overlapping ranges in a file without closing the DAX mode mmap, since then the segments are entirely logical instead of needing separate physical files.

Quoted 8 lines

I note a comment below regarding a per-directly setting, but don't have the background to fully understand what's being suggested. However, I'll note here that I can live with a per-directory granularity, as relinking a file into a new dir is a constant time operation, whilst the move described above isn't. So if a per-directory granularity is easier than a per-file one that's fine, though as a person with only passing knowledge of filesystem design I don't see how having multiple links to a file can work cleanly in that case.

Well, with per-directory setting, relinking the file will not magically make it stop using DAX. So your situation would be very similar to the current one, except "copy to non-DAX device" can be replaced by "copy to non-DAX directory". Maybe the "copy" part could be actually reflink which would make it faster.

Quoted 5 lines

P.S. I'll cheekily take the opportunity of having your attention to tack on one minor gripe about the current system: The only way to know if a mmap with MAP_SYNC will work is to try it and catch the error. Which would be reasonable if it were free of side effects. However, the process requires first expanding the file to at least the size of the desired map, which is

Quoted 8 lines

done non-atomically i.e. is user visible. There are thus nasty race conditions in the cleanup, where after a failed mmap attempt (e.g the device doesn't support DAX), we try to shrink the file back to its original size, but something else has already opened it at its new, larger size. This is not theoretical: I got caught by it whilst adapting some of our middleware to use pmem. Therefore, some way to probe the file path for its capability would be nice, much the same as I can e.g. inspect file permissions to (more or less) evaluate if I can write it without actually mutating it. Thanks!

Well, reporting error on mmap(2) is the only way how to avoid time-to-check-time-to-use races. And these are very important when we are speaking about data integrity guarantees. So that's not going to change. But with Ira's patches you could use statx(2) to check whether file at least supports DAX and so avoid doing mmap check with the side effects in the common case where it's hopeless... I'd also think that you could currently do mmap check with the current file size and if it succeeds, expand the file to the desired size and mmap again. It's not ideal but it should work.

Hi All

I'm a middleware developer, focused on how Java (JVM) workloads can benefit from app-direct mode pmem. Initially the target is apps that need a fast binary log for fault tolerance: the classic database WAL use case; transaction coordination systems; enterprise message bus persistence and suchlike. Critically, there are cases where we use log based storage, i.e. it's not the strict 'read rarely, only on recovery' model that a classic db may have, but more of a 'append only, read many times' event stream model.

Think of the log oriented data storage as having logical segments (let's implement them as files), of which the most recent is being appended to (read_write) and the remaining N-1 older segments are full and sealed, so effectively immutable (read_only) until discarded. The tail segment needs to be in DAX mode for optimal write performance, as the size of the append may be sub-block and we don't want the overhead of the kernel call anyhow. So that's clearly a good fit for putting on a DAX fs mount and using mmap with MAP_SYNC.

However, we want fast read access into the segments, to retrieve stored records. The small access index can be built in volatile RAM (assuming we're willing to take the startup overhead of a full file scan at recovery time) but the data itself is big and we don't want to move it all off pmem. Which means the requirements are now different: we want the O/S cache to pull hot data into fast volatile RAM for us, which DAX explicitly won't do. Effectively a poor man's 'memory mode' pmem, rather than app-direct mode, except here we're using the O/S rather than the hardware memory controller to do the cache management for us.

Currently this requires closing the full (read_write) file, then copying it to a non-DAX device and reopening it (read_only) there. Clearly that's expensive and rather tedious. Instead, I'd like to close the MAP_SYNC mmap, then, leaving the file where it is, reopen it in a mode that will instead go via the O/S cache in the traditional manner.

Bonus points if I can do it
over non-overlapping ranges in a file without closing the DAX mode mmap, since then the segments are entirely logical instead of needing separate physical files.

I note a comment below regarding a per-directly setting, but don't have the background to fully understand what's being suggested. However, I'll note here that I can live with a per-directory granularity, as relinking a file into a new dir is a constant time operation, whilst the move described above isn't. So if a per-directory granularity is easier than a per-file one that's fine, though as a person with only passing knowledge of filesystem design I don't see how having multiple links to a file can work cleanly in that case.

Hope that helps.

Jonathan

P.S. I'll cheekily take the opportunity of having your attention to tack on one minor gripe about the current system: The only way to know if a mmap with MAP_SYNC will work is to try it and catch the error. Which would be reasonable if it were free of side effects. However, the process requires first expanding the file to at least the size of the desired map, which is done non-atomically i.e. is user visible. There are thus nasty race conditions in the cleanup, where after a failed mmap attempt (e.g the device doesn't support DAX), we try to shrink the file back to its original size, but something else has already opened it at its new, larger size. This is not theoretical: I got caught by it whilst adapting some of our middleware to use pmem. Therefore, some way to probe the file path for its capability would be nice, much the same as I can e.g. inspect file permissions to (more or less) evaluate if I can write it without actually mutating it. Thanks!

Quoted 18 lines · Jeff Moyer

Quoted 7 lines · Christoph Hellwig

And my point is that if we ensure S_DAX can only be checked if there are no blocks on the file, is is fairly easy to provide the same guarantee. And I've not heard any argument that we really need more flexibility than that. In fact I think just being able to change it on the parent directory and inheriting the flag might be more than plenty, which would lead to a very simple implementation without any of the crazy overhead in this series.

I know of one user who had at least mentioned it to me, so I cc'd him. Jonathan, can you describe your use case for being able to change a file between dax and non-dax modes? Or, if I'm misremembering, just correct me?

Thanks!
Jeff

Signature · 2 lines

Registered in England and Wales under Company Registration No. 03798903
Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander

Hi All

I'm a middleware developer, focused on how Java (JVM) workloads can benefit from app-direct mode pmem. Initially the target is apps that need a fast binary log for fault tolerance: the classic database WAL use case; transaction coordination systems; enterprise message bus persistence and suchlike. Critically, there are cases where we use log based storage, i.e. it's not the strict 'read rarely, only on recovery' model that a classic db may have, but more of a 'append only, read many times' event stream model.

Think of the log oriented data storage as having logical segments (let's implement them as files), of which the most recent is being appended to (read_write) and the remaining N-1 older segments are full and sealed, so effectively immutable (read_only) until discarded. The tail segment needs to be in DAX mode for optimal write performance, as the size of the append may be sub-block and we don't want the overhead of the kernel call anyhow. So that's clearly a good fit for putting on a DAX fs mount and using mmap with MAP_SYNC.

However, we want fast read access into the segments, to retrieve stored records. The small access index can be built in volatile RAM (assuming we're willing to take the startup overhead of a full file scan at recovery time) but the data itself is big and we don't want to move it all off pmem. Which means the requirements are now different: we want the O/S cache to pull hot data into fast volatile RAM for us, which DAX explicitly won't do. Effectively a poor man's 'memory mode' pmem, rather than app-direct mode, except here we're using the O/S rather than the hardware memory controller to do the cache management for us.

Currently this requires closing the full (read_write) file, then copying it to a non-DAX device and reopening it (read_only) there. Clearly that's expensive and rather tedious. Instead, I'd like to close the MAP_SYNC mmap, then, leaving the file where it is, reopen it in a mode that will instead go via the O/S cache in the traditional manner. Bonus points if I can do it over non-overlapping ranges in a file without closing the DAX mode mmap, since then the segments are entirely logical instead of needing separate physical files.

Quoted 32 lines · Jonathan Halliday

Hi All

I'm a middleware developer, focused on how Java (JVM) workloads can benefit from app-direct mode pmem. Initially the target is apps that need a fast binary log for fault tolerance: the classic database WAL use case; transaction coordination systems; enterprise message bus persistence and suchlike. Critically, there are cases where we use log based storage, i.e. it's not the strict 'read rarely, only on recovery' model that a classic db may have, but more of a 'append only, read many times' event stream model.

Think of the log oriented data storage as having logical segments (let's implement them as files), of which the most recent is being appended to (read_write) and the remaining N-1 older segments are full and sealed, so effectively immutable (read_only) until discarded. The tail segment needs to be in DAX mode for optimal write performance, as the size of the append may be sub-block and we don't want the overhead of the kernel call anyhow. So that's clearly a good fit for putting on a DAX fs mount and using mmap with MAP_SYNC.

However, we want fast read access into the segments, to retrieve stored records. The small access index can be built in volatile RAM (assuming we're willing to take the startup overhead of a full file scan at recovery time) but the data itself is big and we don't want to move it all off pmem. Which means the requirements are now different: we want the O/S cache to pull hot data into fast volatile RAM for us, which DAX explicitly won't do. Effectively a poor man's 'memory mode' pmem, rather than app-direct mode, except here we're using the O/S rather than the hardware memory controller to do the cache management for us.

Quoted 8 lines

Currently this requires closing the full (read_write) file, then copying it to a non-DAX device and reopening it (read_only) there. Clearly that's expensive and rather tedious. Instead, I'd like to close the MAP_SYNC mmap, then, leaving the file where it is, reopen it in a mode that will instead go via the O/S cache in the traditional manner. Bonus points if I can do it over non-overlapping ranges in a file without closing the DAX mode mmap, since then the segments are entirely logical instead of needing separate physical files.

Hi John,

IIRC we chatted about this at PIRL, right?

At the time it sounded more like mixed mode dax, i.e. dax writes, but cached reads. To me that's an optimization to optionally use dax for direct-I/O writes, with its existing set of page-cache coherence warts, and not a capability to dynamically switch the dax-mode. mmap+MAP_SYNC seems the wrong interface for this. This writeup mentions bypassing kernel call overhead, but I don't see how a dax-write syscall is cheaper than an mmap syscall plus fault. If direct-I/O to a dax capable file bypasses the block layer, isn't that about the maximum of kernel overhead that can be cut out of this use case? Otherwise MAP_SYNC is a facility to achieve efficient sub-block update-in-place writes not append writes.

Quoted 47 lines · Dan Williams

Quoted 32 lines · Jonathan Halliday

Hi All

I'm a middleware developer, focused on how Java (JVM) workloads can benefit from app-direct mode pmem. Initially the target is apps that need a fast binary log for fault tolerance: the classic database WAL use case; transaction coordination systems; enterprise message bus persistence and suchlike. Critically, there are cases where we use log based storage, i.e. it's not the strict 'read rarely, only on recovery' model that a classic db may have, but more of a 'append only, read many times' event stream model.

Think of the log oriented data storage as having logical segments (let's implement them as files), of which the most recent is being appended to (read_write) and the remaining N-1 older segments are full and sealed, so effectively immutable (read_only) until discarded. The tail segment needs to be in DAX mode for optimal write performance, as the size of the append may be sub-block and we don't want the overhead of the kernel call anyhow. So that's clearly a good fit for putting on a DAX fs mount and using mmap with MAP_SYNC.

However, we want fast read access into the segments, to retrieve stored records. The small access index can be built in volatile RAM (assuming we're willing to take the startup overhead of a full file scan at recovery time) but the data itself is big and we don't want to move it all off pmem. Which means the requirements are now different: we want the O/S cache to pull hot data into fast volatile RAM for us, which DAX explicitly won't do. Effectively a poor man's 'memory mode' pmem, rather than app-direct mode, except here we're using the O/S rather than the hardware memory controller to do the cache management for us.

Quoted 8 lines

Currently this requires closing the full (read_write) file, then copying it to a non-DAX device and reopening it (read_only) there. Clearly that's expensive and rather tedious. Instead, I'd like to close the MAP_SYNC mmap, then, leaving the file where it is, reopen it in a mode that will instead go via the O/S cache in the traditional manner. Bonus points if I can do it over non-overlapping ranges in a file without closing the DAX mode mmap, since then the segments are entirely logical instead of needing separate physical files.

Hi John,

IIRC we chatted about this at PIRL, right?

Quoted 11 lines

At the time it sounded more like mixed mode dax, i.e. dax writes, but cached reads. To me that's an optimization to optionally use dax for direct-I/O writes, with its existing set of page-cache coherence warts, and not a capability to dynamically switch the dax-mode. mmap+MAP_SYNC seems the wrong interface for this. This writeup mentions bypassing kernel call overhead, but I don't see how a dax-write syscall is cheaper than an mmap syscall plus fault. If direct-I/O to a dax capable file bypasses the block layer, isn't that about the maximum of kernel overhead that can be cut out of this use case? Otherwise MAP_SYNC is a facility to achieve efficient sub-block update-in-place writes not append writes.

Well, even for appends you'll pay the cost only once per page (or maybe even once per huge page) when using MAP_SYNC. With a syscall you'll pay once per write. So although it would be good to check real numbers, the design isn't non-sensical to me.

Quoted 11 lines · Dave Chinner

Quoted 7 lines

No, the original code was broken because it didn't serialize between DAX and buffer access.

Take a step back and look where the problems are, and they are not mostly with the aops. In fact the only aop useful for DAX is is ->writepages, and how it uses ->writepages is actually a bit of an abuse of that interface.

The races are all through the fops, too, which is one of the reasons Darrick mentioned we should probably move this up to file ops level...

But the file ops are very simple to use. Pass the flag in the iocb, and make sure the flag can only changed with i_rwsem held. That part is pretty trivial, the interesting case is mmap because it is so spread out.

Quoted 5 lines

Quoted 2 lines

So what we really need it just a way to prevent switching the flag when a file is mapped,

That's not sufficient.

Quoted 5 lines

We also have to prevent the file from being mapped *while we are switching*. Nothing in the mmap() path actually serialises against filesystem operations, and the initial behavioural checks in the page fault path are similarly unserialised against changing the S_DAX flag.

And the important part here is ->mmap. If ->mmap doesn't get through we are not going to see page faults.

Quoted 11 lines

Quoted 6 lines

and in the normal read/write path ensure the
flag can't be switch while I/O is going on, which could either be done by ensuring it is only switched under i_rwsem or equivalent protection, or by setting the DAX flag once in the iocb similar to IOCB_DIRECT.

The iocb path is not the problem - that's entirely serialised against S_DAX changes by the i_rwsem. The problem is that we have no equivalent filesystem level serialisation for the entire mmap/page fault path, and it checks S_DAX all over the place.

Not right now. We have various IS_DAX checks outside it. But it is easily fixable indeed.

Quoted 6 lines

/me wonders if the best thing to do is to add a ->fault callout to tell the filesystem to lock/unlock the inode right up at the top of the page fault path, outside even the mmap_sem. That means all the methods that the page fault calls are protected against S_DAX changes, and it gives us a low cost method of serialising page faults against DIO (e.g. via inode_dio_wait())....

Maybe. Especially if it solves real problems and isn't just new overhead to add an esoteric feature.

Quoted 10 lines

Quoted 5 lines

And they easiest way to get all this done is as a first step to just allowing switching the flag when no blocks are allocated at all, similar to how the rt flag works.

False equivalence - it is not similar because the RT flag changes and their associated state checks are *already fully serialised* by the XFS_ILOCK_EXCL. S_DAX accesses have no such serialisation, and that's the problem we need to solve...

And my point is that if we ensure S_DAX can only be checked if there are no blocks on the file, is is fairly easy to provide the same guarantee. And I've not heard any argument that we really need more flexibility than that. In fact I think just being able to change it on the parent directory and inheriting the flag might be more than plenty, which would lead to a very simple implementation without any of the crazy overhead in this series.

/me wonders if the best thing to do is to add a ->fault callout to tell the filesystem to lock/unlock the inode right up at the top of the page fault path, outside even the mmap_sem. That means all the methods that the page fault calls are protected against S_DAX changes, and it gives us a low cost method of serialising page faults against DIO (e.g. via inode_dio_wait())....

Quoted 6 lines · Dave Chinner

/me wonders if the best thing to do is to add a ->fault callout to tell the filesystem to lock/unlock the inode right up at the top of the page fault path, outside even the mmap_sem. That means all the methods that the page fault calls are protected against S_DAX changes, and it gives us a low cost method of serialising page faults against DIO (e.g. via inode_dio_wait())....

Well, that's going to be pretty hard. The main problem is: you cannot lookup VMA until you hold mmap_sem and the inode is inside the VMA. And this is a fundamental problem because until you hold mmap_sem, the address space can change and thus the virtual address you are faulting can be changing inode it is mapped to. So you would have to do some dance like:

lock mmap_sem
lookup vma
get inode reference
drop mmap_sem
tell fs about page fault
lock mmap_sem
is the vma still the same?

And I'm pretty confident the overhead will be visible in page fault intensive workloads...

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

From: Ira Weiny <[email protected]>

DAX requires special address space operations (aops). Changing DAX state therefore requires changing those aops.

However, many functions require aops to remain consistent through a deep call stack.

Define a vfs level inode rwsem to protect aops throughout call stacks which require them.

Finally, define calls to be used in subsequent patches when aops usage needs to be quiesced by the file system.

Signed-off-by Ira Weiny <[email protected]>

---
Changes from V3:
Convert from global to a per-inode rwsem
Remove pre-optimization
Remove static branch stuff
Change function names from inode_dax_state_* to inode_aops_*
I kept 'inode' as the synchronization is at the inode level now (probably where it belongs)...

and I still prefer *_[down|up]_[read|write] as those names better reflect the use and interaction between users (readers) and writers better. 'XX_start_aop()' would have to be matched with something like 'XX_wait_for_aop_user()' and 'XX_release_aop_users()' or something which does not make sense on the 'writer' side.

Changes from V2

Rebase on linux-next-08-02-2020

Fix locking order
Change all references from mode to state where appropriate add CONFIG_FS_DAX requirement for state change
Use a static branch to enable locking only when a dax capable
device has been seen.

Move the lock to a global vfs lock

this does a few things
1) preps us better for ext4 support
2) removes funky callbacks from inode ops
3) remove complexity from XFS and probably from ext4 later

We can do this because
1) the locking order is required to be at the highest level anyway, so why complicate xfs
2) We had to move the sem to the super_block because it is too heavy for the inode.
3) After internal discussions with Dan we decided that this would be easier, just as performant, and with slightly less overhead than in the VFS SB.

We also change the functions names to up/down; read/write as appropriate. Previous names were over simplified.

Update comments and documentation

squash: add locking

---
 Documentation/filesystems/vfs.rst | 16 ++++++++
 fs/attr.c                         |  1 +
 fs/inode.c                        | 15 +++++--
 fs/iomap/buffered-io.c            |  1 +
 fs/open.c                         |  4 ++
 fs/stat.c                         |  2 +
 fs/xfs/xfs_icache.c               |  1 +
 include/linux/fs.h                | 66 ++++++++++++++++++++++++++++++-
 mm/fadvise.c                      |  7 +++-
 mm/filemap.c                      |  4 ++
 mm/huge_memory.c                  |  1 +
 mm/khugepaged.c                   |  2 +
 mm/util.c                         |  9 ++++-
 13 files changed, 121 insertions(+), 8 deletions(-)

Documentation/filesystems/vfs.rst +16 -0

index 7d4d09dd5e6d..4a10a232f8e2 100644
@@ -934,6 +934,22 @@ cache in your filesystem.  The following members are defined:
 	Called during swapoff on files where swap_activate was
 	successful.
 
+Changing DAX 'state' dynamically
+----------------------------------
+
+Some file systems which support DAX want to be able to change the DAX state
+dyanically.  To switch the state safely we lock the inode state in all "normal"
+file system operations and restrict state changes to those operations.  The
+specific rules are.
+
+        1) the direct_IO address_space_operation must be supported in all
+           potential a_ops vectors for any state suported by the inode.
+
+        3) DAX state changes shall not be allowed while the file is mmap'ed
+        4) For non-mmaped operations the VFS layer must take the read lock for any
+           use of IS_DAX()
+        5) Filesystems take the write lock when changing DAX states.
+
 
 The File Object
 ===============

fs/attr.c +1 -0

index b4bbdbd4c8ca..9b15f73d1079 100644
@@ -332,6 +332,7 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de
 	if (error)
 		return error;
 
+	/* DAX read state should already be held here */
 	if (inode->i_op->setattr)
 		error = inode->i_op->setattr(dentry, attr);
 	else

fs/inode.c +12 -3

index 7d57068b6b7a..6e4f1cc872f2 100644
@@ -200,6 +200,7 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
 #endif
 	inode->i_flctx = NULL;
 	this_cpu_inc(nr_inodes);
+	init_rwsem(&inode->i_aops_sem);
 
 	return 0;
 out:

@@ -1616,11 +1617,19 @@ EXPORT_SYMBOL(iput);
  */
 int bmap(struct inode *inode, sector_t *block)
 {
-	if (!inode->i_mapping->a_ops->bmap)
-		return -EINVAL;
+	int ret = 0;
+
+	inode_aops_down_read(inode);
+	if (!inode->i_mapping->a_ops->bmap) {
+		ret = -EINVAL;
+		goto err;
+	}
 
 	*block = inode->i_mapping->a_ops->bmap(inode->i_mapping, *block);
-	return 0;
+
+err:
+	inode_aops_up_read(inode);
+	return ret;
 }
 EXPORT_SYMBOL(bmap);
 #endif

fs/iomap/buffered-io.c +1 -0

index 7c84c4c027c4..e313a34d5fa6 100644
@@ -999,6 +999,7 @@ iomap_zero_range_actor(struct inode *inode, loff_t pos, loff_t count,
 		offset = offset_in_page(pos);
 		bytes = min_t(loff_t, PAGE_SIZE - offset, count);
 
+		/* DAX state read should already be held here */
 		if (IS_DAX(inode))
 			status = iomap_dax_zero(pos, offset, bytes, iomap);
 		else

fs/open.c +4 -0

index 0788b3715731..3abf0bfac462 100644
@@ -59,10 +59,12 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
 	if (ret)
 		newattrs.ia_valid |= ret | ATTR_FORCE;
 
+	inode_aops_down_read(dentry->d_inode);
 	inode_lock(dentry->d_inode);
 	/* Note any delegations or leases have already been broken: */
 	ret = notify_change(dentry, &newattrs, NULL);
 	inode_unlock(dentry->d_inode);
+	inode_aops_up_read(dentry->d_inode);
 	return ret;
 }
 

@@ -306,7 +308,9 @@ int vfs_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 		return -EOPNOTSUPP;
 
 	file_start_write(file);
+	inode_aops_down_read(inode);
 	ret = file->f_op->fallocate(file, mode, offset, len);
+	inode_aops_up_read(inode);
 
 	/*
 	 * Create inotify and fanotify events.

fs/stat.c +2 -0

index 894699c74dde..274b3ccc82b1 100644
@@ -79,8 +79,10 @@ int vfs_getattr_nosec(const struct path *path, struct kstat *stat,
 	if (IS_AUTOMOUNT(inode))
 		stat->attributes |= STATX_ATTR_AUTOMOUNT;
 
+	inode_aops_down_read(inode);
 	if (IS_DAX(inode))
 		stat->attributes |= STATX_ATTR_DAX;
+	inode_aops_up_read(inode);
 
 	if (inode->i_op->getattr)
 		return inode->i_op->getattr(path, stat, request_mask,

fs/xfs/xfs_icache.c +1 -0

index 836a1f09be03..3e83a97dc047 100644
@@ -420,6 +420,7 @@ xfs_iget_cache_hit(
 		rcu_read_unlock();
 
 		ASSERT(!rwsem_is_locked(&inode->i_rwsem));
+		ASSERT(!rwsem_is_locked(&inode->i_aops_sem));
 		error = xfs_reinit_inode(mp, inode);
 		if (error) {
 			bool wake;

include/linux/fs.h +16 -0

index 63d1e533a07d..ad0f2368031b 100644
@@ -40,6 +40,7 @@
 #include <linux/fs_types.h>
 #include <linux/build_bug.h>
 #include <linux/stddef.h>
+#include <linux/percpu-rwsem.h>
 
 #include <asm/byteorder.h>
 #include <uapi/linux/fs.h>

@@ -359,6 +360,11 @@ typedef struct {
 typedef int (*read_actor_t)(read_descriptor_t *, struct page *,
 		unsigned long, unsigned long);
 
+/**
+ * NOTE: DO NOT define new functions in address_space_operations without first
+ * considering how dynamic DAX states are to be supported.  See the
+ * inode_aops_*_read() functions
+ */
 struct address_space_operations {
 	int (*writepage)(struct page *page, struct writeback_control *wbc);
 	int (*readpage)(struct file *, struct page *);

@@ -735,6 +741,9 @@ struct inode {
 #endif
 
 	void			*i_private; /* fs or device private pointer */
+#if defined(CONFIG_FS_DAX)
+	struct rw_semaphore	i_aops_sem;
+#endif
 } __randomize_layout;
 
 struct timespec64 timestamp_truncate(struct timespec64 t, struct inode *inode);

@@ -1817,6 +1826,11 @@ struct block_device_operations;
 
 struct iov_iter;
 
+/**
+ * NOTE: DO NOT define new functions in file_operations without first
+ * considering how dynamic address_space operations are to be supported.  See
+ * the inode_aops_*_read() functions in this file.
+ */
 struct file_operations {
 	struct module *owner;
 	loff_t (*llseek) (struct file *, loff_t, int);

@@ -1889,16 +1903,64 @@ struct inode_operations {
 	int (*set_acl)(struct inode *, struct posix_acl *, int);
 } ____cacheline_aligned;
 
+#if defined(CONFIG_FS_DAX)
+/*

include/linux/fs.h +38 -2

+ */
+static inline void inode_aops_down_read(struct inode *inode)
+{
+	down_read(&inode->i_aops_sem);
+}
+static inline void inode_aops_up_read(struct inode *inode)
+{
+	up_read(&inode->i_aops_sem);
+}
+static inline void inode_aops_down_write(struct inode *inode)
+{
+	down_write(&inode->i_aops_sem);
+}
+static inline void inode_aops_up_write(struct inode *inode)
+{
+	up_write(&inode->i_aops_sem);
+}
+#else /* !CONFIG_FS_DAX */
+#define inode_aops_down_read(inode) do { (void)(inode); } while (0)
+#define inode_aops_up_read(inode) do { (void)(inode); } while (0)
+#define inode_aops_down_write(inode) do { (void)(inode); } while (0)
+#define inode_aops_up_write(inode) do { (void)(inode); } while (0)
+#endif /* CONFIG_FS_DAX */
+
 static inline ssize_t call_read_iter(struct file *file, struct kiocb *kio,
 				     struct iov_iter *iter)
 {
-	return file->f_op->read_iter(kio, iter);
+	struct inode		*inode = file_inode(kio->ki_filp);
+	ssize_t ret;
+
+	inode_aops_down_read(inode);
+	ret = file->f_op->read_iter(kio, iter);
+	inode_aops_up_read(inode);
+	return ret;
 }
 
 static inline ssize_t call_write_iter(struct file *file, struct kiocb *kio,
 				      struct iov_iter *iter)
 {
-	return file->f_op->write_iter(kio, iter);
+	struct inode		*inode = file_inode(kio->ki_filp);
+	ssize_t ret;
+
+	inode_aops_down_read(inode);
+	ret = file->f_op->write_iter(kio, iter);
+	inode_aops_up_read(inode);
+	return ret;
 }
 
 static inline int call_mmap(struct file *file, struct vm_area_struct *vma)

mm/fadvise.c +5 -2

index 4f17c83db575..6a30febb11e0 100644
@@ -48,6 +48,8 @@ int generic_fadvise(struct file *file, loff_t offset, loff_t len, int advice)
 	bdi = inode_to_bdi(mapping->host);
 
 	if (IS_DAX(inode) || (bdi == &noop_backing_dev_info)) {
+		int ret = 0;
+
 		switch (advice) {
 		case POSIX_FADV_NORMAL:
 		case POSIX_FADV_RANDOM:

@@ -58,9 +60,10 @@ int generic_fadvise(struct file *file, loff_t offset, loff_t len, int advice)
 			/* no bad return value, but ignore advice */
 			break;
 		default:
-			return -EINVAL;
+			ret = -EINVAL;
 		}
-		return 0;
+
+		return ret;
 	}
 
 	/*

mm/filemap.c +4 -0

index 1784478270e1..3a7863ba51b9 100644
@@ -2293,6 +2293,8 @@ generic_file_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 		 * and return.  Otherwise fallthrough to buffered io for
 		 * the rest of the read.  Buffered reads will not work for
 		 * DAX files, so don't bother trying.
+		 *
+		 * IS_DAX is protected under ->read_iter lock
 		 */
 		if (retval < 0 || !count || iocb->ki_pos >= size ||
 		    IS_DAX(inode))

@@ -3377,6 +3379,8 @@ ssize_t __generic_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 		 * holes, for example.  For DAX files, a buffered write will
 		 * not succeed (even if it did, DAX does not handle dirty
 		 * page-cache pages correctly).
+		 *
+		 * IS_DAX is protected under ->write_iter lock
 		 */
 		if (written < 0 || !iov_iter_count(from) || IS_DAX(inode))
 			goto out;

mm/huge_memory.c +1 -0

index b08b199f9a11..3d05bd10d83e 100644
@@ -572,6 +572,7 @@ unsigned long thp_get_unmapped_area(struct file *filp, unsigned long addr,
 	unsigned long ret;
 	loff_t off = (loff_t)pgoff << PAGE_SHIFT;
 
+	/* Should not need locking here because mmap is not allowed */
 	if (!IS_DAX(filp->f_mapping->host) || !IS_ENABLED(CONFIG_FS_DAX_PMD))
 		goto out;
 

mm/khugepaged.c +2 -0

index b679908743cb..f048178e2b93 100644
@@ -1592,9 +1592,11 @@ static void collapse_file(struct mm_struct *mm,
 		} else {	/* !is_shmem */
 			if (!page || xa_is_value(page)) {
 				xas_unlock_irq(&xas);
+				inode_aops_down_read(file->f_inode);
 				page_cache_sync_readahead(mapping, &file->f_ra,
 							  file, index,
 							  PAGE_SIZE);
+				inode_aops_up_read(file->f_inode);
 				/* drain pagevecs to help isolate_lru_page() */
 				lru_add_drain();
 				page = find_lock_page(mapping, index);

mm/util.c +8 -1

index 988d11e6c17c..a4fb0670137d 100644
@@ -501,11 +501,18 @@ unsigned long vm_mmap_pgoff(struct file *file, unsigned long addr,
 
 	ret = security_mmap_file(file, prot, flag);
 	if (!ret) {
-		if (down_write_killable(&mm->mmap_sem))
+		if (file)
+			inode_aops_down_read(file_inode(file));
+		if (down_write_killable(&mm->mmap_sem)) {
+			if (file)
+				inode_aops_up_read(file_inode(file));
 			return -EINTR;
+		}
 		ret = do_mmap_pgoff(file, addr, len, prot, flag, pgoff,
 				    &populate, &uf);
 		up_write(&mm->mmap_sem);
+		if (file)
+			inode_aops_up_read(file_inode(file));
 		userfaultfd_unmap_complete(mm, &uf);
 		if (populate)
 			mm_populate(ret, populate);

Signature · 1 line

2.21.0

Quoted 274 lines · On Thu, Feb 20, 2020 at 04:41:28PM -0800, [email protected] wrote:

From: Ira Weiny <[email protected]>

DAX requires special address space operations (aops). Changing DAX state therefore requires changing those aops.

However, many functions require aops to remain consistent through a deep call stack.

Define a vfs level inode rwsem to protect aops throughout call stacks which require them.

Finally, define calls to be used in subsequent patches when aops usage needs to be quiesced by the file system.

Signed-off-by Ira Weiny <[email protected]>

---
Changes from V3:
Convert from global to a per-inode rwsem
Remove pre-optimization
Remove static branch stuff
Change function names from inode_dax_state_* to inode_aops_*
I kept 'inode' as the synchronization is at the inode level now (probably where it belongs)...

and I still prefer *_[down|up]_[read|write] as those names better reflect the use and interaction between users (readers) and writers better. 'XX_start_aop()' would have to be matched with something like 'XX_wait_for_aop_user()' and 'XX_release_aop_users()' or something which does not make sense on the 'writer' side.

Changes from V2

Rebase on linux-next-08-02-2020

Fix locking order
Change all references from mode to state where appropriate add CONFIG_FS_DAX requirement for state change
Use a static branch to enable locking only when a dax capable
device has been seen.

Move the lock to a global vfs lock

this does a few things
1) preps us better for ext4 support
2) removes funky callbacks from inode ops
3) remove complexity from XFS and probably from ext4 later

We can do this because
1) the locking order is required to be at the highest level anyway, so why complicate xfs
2) We had to move the sem to the super_block because it is too heavy for the inode.
3) After internal discussions with Dan we decided that this would be easier, just as performant, and with slightly less overhead than in the VFS SB.

We also change the functions names to up/down; read/write as appropriate. Previous names were over simplified.

Update comments and documentation

squash: add locking

---
 Documentation/filesystems/vfs.rst | 16 ++++++++
 fs/attr.c                         |  1 +
 fs/inode.c                        | 15 +++++--
 fs/iomap/buffered-io.c            |  1 +
 fs/open.c                         |  4 ++
 fs/stat.c                         |  2 +
 fs/xfs/xfs_icache.c               |  1 +
 include/linux/fs.h                | 66 ++++++++++++++++++++++++++++++-
 mm/fadvise.c                      |  7 +++-
 mm/filemap.c                      |  4 ++
 mm/huge_memory.c                  |  1 +
 mm/khugepaged.c                   |  2 +
 mm/util.c                         |  9 ++++-
 13 files changed, 121 insertions(+), 8 deletions(-)

Documentation/filesystems/vfs.rst +16 -0

index 7d4d09dd5e6d..4a10a232f8e2 100644
@@ -934,6 +934,22 @@ cache in your filesystem.  The following members are defined:
 	Called during swapoff on files where swap_activate was
 	successful.
 
+Changing DAX 'state' dynamically
+----------------------------------
+
+Some file systems which support DAX want to be able to change the DAX state
+dyanically.  To switch the state safely we lock the inode state in all "normal"
+file system operations and restrict state changes to those operations.  The
+specific rules are.
+
+        1) the direct_IO address_space_operation must be supported in all
+           potential a_ops vectors for any state suported by the inode.
+
+        3) DAX state changes shall not be allowed while the file is mmap'ed
+        4) For non-mmaped operations the VFS layer must take the read lock for any
+           use of IS_DAX()
+        5) Filesystems take the write lock when changing DAX states.
+
 
 The File Object
 ===============

fs/attr.c +1 -0

index b4bbdbd4c8ca..9b15f73d1079 100644
@@ -332,6 +332,7 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de
 	if (error)
 		return error;
 
+	/* DAX read state should already be held here */
 	if (inode->i_op->setattr)
 		error = inode->i_op->setattr(dentry, attr);
 	else

fs/inode.c +12 -3

index 7d57068b6b7a..6e4f1cc872f2 100644
@@ -200,6 +200,7 @@ int inode_init_always(struct super_block *sb, struct inode *inode)
 #endif
 	inode->i_flctx = NULL;
 	this_cpu_inc(nr_inodes);
+	init_rwsem(&inode->i_aops_sem);
 
 	return 0;
 out:

@@ -1616,11 +1617,19 @@ EXPORT_SYMBOL(iput);
  */
 int bmap(struct inode *inode, sector_t *block)
 {
-	if (!inode->i_mapping->a_ops->bmap)
-		return -EINVAL;
+	int ret = 0;
+
+	inode_aops_down_read(inode);
+	if (!inode->i_mapping->a_ops->bmap) {
+		ret = -EINVAL;
+		goto err;
+	}
 
 	*block = inode->i_mapping->a_ops->bmap(inode->i_mapping, *block);
-	return 0;
+
+err:
+	inode_aops_up_read(inode);
+	return ret;
 }
 EXPORT_SYMBOL(bmap);
 #endif

fs/iomap/buffered-io.c +1 -0

index 7c84c4c027c4..e313a34d5fa6 100644
@@ -999,6 +999,7 @@ iomap_zero_range_actor(struct inode *inode, loff_t pos, loff_t count,
 		offset = offset_in_page(pos);
 		bytes = min_t(loff_t, PAGE_SIZE - offset, count);
 
+		/* DAX state read should already be held here */
 		if (IS_DAX(inode))
 			status = iomap_dax_zero(pos, offset, bytes, iomap);
 		else

fs/open.c +4 -0

index 0788b3715731..3abf0bfac462 100644
@@ -59,10 +59,12 @@ int do_truncate(struct dentry *dentry, loff_t length, unsigned int time_attrs,
 	if (ret)
 		newattrs.ia_valid |= ret | ATTR_FORCE;
 
+	inode_aops_down_read(dentry->d_inode);
 	inode_lock(dentry->d_inode);
 	/* Note any delegations or leases have already been broken: */
 	ret = notify_change(dentry, &newattrs, NULL);
 	inode_unlock(dentry->d_inode);
+	inode_aops_up_read(dentry->d_inode);
 	return ret;
 }
 

@@ -306,7 +308,9 @@ int vfs_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 		return -EOPNOTSUPP;
 
 	file_start_write(file);
+	inode_aops_down_read(inode);
 	ret = file->f_op->fallocate(file, mode, offset, len);
+	inode_aops_up_read(inode);
 
 	/*
 	 * Create inotify and fanotify events.

fs/stat.c +2 -0

index 894699c74dde..274b3ccc82b1 100644
@@ -79,8 +79,10 @@ int vfs_getattr_nosec(const struct path *path, struct kstat *stat,
 	if (IS_AUTOMOUNT(inode))
 		stat->attributes |= STATX_ATTR_AUTOMOUNT;
 
+	inode_aops_down_read(inode);
 	if (IS_DAX(inode))
 		stat->attributes |= STATX_ATTR_DAX;
+	inode_aops_up_read(inode);
 
 	if (inode->i_op->getattr)
 		return inode->i_op->getattr(path, stat, request_mask,

fs/xfs/xfs_icache.c +1 -0

index 836a1f09be03..3e83a97dc047 100644
@@ -420,6 +420,7 @@ xfs_iget_cache_hit(
 		rcu_read_unlock();
 
 		ASSERT(!rwsem_is_locked(&inode->i_rwsem));
+		ASSERT(!rwsem_is_locked(&inode->i_aops_sem));
 		error = xfs_reinit_inode(mp, inode);
 		if (error) {
 			bool wake;

include/linux/fs.h +14 -0

index 63d1e533a07d..ad0f2368031b 100644
@@ -40,6 +40,7 @@
 #include <linux/fs_types.h>
 #include <linux/build_bug.h>
 #include <linux/stddef.h>
+#include <linux/percpu-rwsem.h>
 
 #include <asm/byteorder.h>
 #include <uapi/linux/fs.h>

@@ -359,6 +360,11 @@ typedef struct {
 typedef int (*read_actor_t)(read_descriptor_t *, struct page *,
 		unsigned long, unsigned long);
 
+/**
+ * NOTE: DO NOT define new functions in address_space_operations without first
+ * considering how dynamic DAX states are to be supported.  See the
+ * inode_aops_*_read() functions
+ */
 struct address_space_operations {
 	int (*writepage)(struct page *page, struct writeback_control *wbc);
 	int (*readpage)(struct file *, struct page *);

@@ -735,6 +741,9 @@ struct inode {
 #endif
 
 	void			*i_private; /* fs or device private pointer */
+#if defined(CONFIG_FS_DAX)
+	struct rw_semaphore	i_aops_sem;
+#endif
 } __randomize_layout;
 
 struct timespec64 timestamp_truncate(struct timespec64 t, struct inode *inode);

@@ -1817,6 +1826,11 @@ struct block_device_operations;
 
 struct iov_iter;
 
+/**
+ * NOTE: DO NOT define new functions in file_operations without first
+ * considering how dynamic address_space operations are to be supported.  See
+ * the inode_aops_*_read() functions in this file.
+ */
 struct file_operations {
 	struct module *owner;
 	loff_t (*llseek) (struct file *, loff_t, int);

@@ -1889,16 +1903,64 @@ struct inode_operations {
 	int (*set_acl)(struct inode *, struct posix_acl *, int);
 } ____cacheline_aligned;
 

include/linux/fs.h struct inode_operations { · line 1917

Quoted 12 lines

include/linux/fs.h +12 -0

+#if defined(CONFIG_FS_DAX)
+/*
+ * Filesystems wishing to support dynamic DAX states must do the following.
+ *
+ * 1) the direct_IO address_space_operation must be supported in all potential
+ *    a_ops vectors for any state suported by the inode.  This is because the
+ *    direct_IO function is used as a flag long before the function is called.
+
+ * 3) DAX state changes shall not be allowed while the file is mmap'ed
+ * 4) For non-mmaped operations the VFS layer must take the read lock for any
+ *    use of IS_DAX()
+ * 5) Filesystems take the write lock when changing DAX states.

Do you envision ext4 migrating their aops changing code to use i_aops_sem in the future?

--D

Quoted 154 lines

include/linux/fs.h +38 -2

+ */
+static inline void inode_aops_down_read(struct inode *inode)
+{
+	down_read(&inode->i_aops_sem);
+}
+static inline void inode_aops_up_read(struct inode *inode)
+{
+	up_read(&inode->i_aops_sem);
+}
+static inline void inode_aops_down_write(struct inode *inode)
+{
+	down_write(&inode->i_aops_sem);
+}
+static inline void inode_aops_up_write(struct inode *inode)
+{
+	up_write(&inode->i_aops_sem);
+}
+#else /* !CONFIG_FS_DAX */
+#define inode_aops_down_read(inode) do { (void)(inode); } while (0)
+#define inode_aops_up_read(inode) do { (void)(inode); } while (0)
+#define inode_aops_down_write(inode) do { (void)(inode); } while (0)
+#define inode_aops_up_write(inode) do { (void)(inode); } while (0)
+#endif /* CONFIG_FS_DAX */
+
 static inline ssize_t call_read_iter(struct file *file, struct kiocb *kio,
 				     struct iov_iter *iter)
 {
-	return file->f_op->read_iter(kio, iter);
+	struct inode		*inode = file_inode(kio->ki_filp);
+	ssize_t ret;
+
+	inode_aops_down_read(inode);
+	ret = file->f_op->read_iter(kio, iter);
+	inode_aops_up_read(inode);
+	return ret;
 }
 
 static inline ssize_t call_write_iter(struct file *file, struct kiocb *kio,
 				      struct iov_iter *iter)
 {
-	return file->f_op->write_iter(kio, iter);
+	struct inode		*inode = file_inode(kio->ki_filp);
+	ssize_t ret;
+
+	inode_aops_down_read(inode);
+	ret = file->f_op->write_iter(kio, iter);
+	inode_aops_up_read(inode);
+	return ret;
 }
 
 static inline int call_mmap(struct file *file, struct vm_area_struct *vma)

mm/fadvise.c +5 -2

index 4f17c83db575..6a30febb11e0 100644
@@ -48,6 +48,8 @@ int generic_fadvise(struct file *file, loff_t offset, loff_t len, int advice)
 	bdi = inode_to_bdi(mapping->host);
 
 	if (IS_DAX(inode) || (bdi == &noop_backing_dev_info)) {
+		int ret = 0;
+
 		switch (advice) {
 		case POSIX_FADV_NORMAL:
 		case POSIX_FADV_RANDOM:

@@ -58,9 +60,10 @@ int generic_fadvise(struct file *file, loff_t offset, loff_t len, int advice)
 			/* no bad return value, but ignore advice */
 			break;
 		default:
-			return -EINVAL;
+			ret = -EINVAL;
 		}
-		return 0;
+
+		return ret;
 	}
 
 	/*

mm/filemap.c +4 -0

index 1784478270e1..3a7863ba51b9 100644
@@ -2293,6 +2293,8 @@ generic_file_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 		 * and return.  Otherwise fallthrough to buffered io for
 		 * the rest of the read.  Buffered reads will not work for
 		 * DAX files, so don't bother trying.
+		 *
+		 * IS_DAX is protected under ->read_iter lock
 		 */
 		if (retval < 0 || !count || iocb->ki_pos >= size ||
 		    IS_DAX(inode))

@@ -3377,6 +3379,8 @@ ssize_t __generic_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
 		 * holes, for example.  For DAX files, a buffered write will
 		 * not succeed (even if it did, DAX does not handle dirty
 		 * page-cache pages correctly).
+		 *
+		 * IS_DAX is protected under ->write_iter lock
 		 */
 		if (written < 0 || !iov_iter_count(from) || IS_DAX(inode))
 			goto out;

mm/huge_memory.c +1 -0

index b08b199f9a11..3d05bd10d83e 100644
@@ -572,6 +572,7 @@ unsigned long thp_get_unmapped_area(struct file *filp, unsigned long addr,
 	unsigned long ret;
 	loff_t off = (loff_t)pgoff << PAGE_SHIFT;
 
+	/* Should not need locking here because mmap is not allowed */
 	if (!IS_DAX(filp->f_mapping->host) || !IS_ENABLED(CONFIG_FS_DAX_PMD))
 		goto out;
 

mm/khugepaged.c +2 -0

index b679908743cb..f048178e2b93 100644
@@ -1592,9 +1592,11 @@ static void collapse_file(struct mm_struct *mm,
 		} else {	/* !is_shmem */
 			if (!page || xa_is_value(page)) {
 				xas_unlock_irq(&xas);
+				inode_aops_down_read(file->f_inode);
 				page_cache_sync_readahead(mapping, &file->f_ra,
 							  file, index,
 							  PAGE_SIZE);
+				inode_aops_up_read(file->f_inode);
 				/* drain pagevecs to help isolate_lru_page() */
 				lru_add_drain();
 				page = find_lock_page(mapping, index);

mm/util.c +8 -1

index 988d11e6c17c..a4fb0670137d 100644
@@ -501,11 +501,18 @@ unsigned long vm_mmap_pgoff(struct file *file, unsigned long addr,
 
 	ret = security_mmap_file(file, prot, flag);
 	if (!ret) {
-		if (down_write_killable(&mm->mmap_sem))
+		if (file)
+			inode_aops_down_read(file_inode(file));
+		if (down_write_killable(&mm->mmap_sem)) {
+			if (file)
+				inode_aops_up_read(file_inode(file));
 			return -EINTR;
+		}
 		ret = do_mmap_pgoff(file, addr, len, prot, flag, pgoff,
 				    &populate, &uf);
 		up_write(&mm->mmap_sem);
+		if (file)
+			inode_aops_up_read(file_inode(file));
 		userfaultfd_unmap_complete(mm, &uf);
 		if (populate)
 			mm_populate(ret, populate);

Signature · 1 line

2.21.0

From: Ira Weiny <[email protected]>

XFS requires the use of the aops of an inode to quiesced prior to changing it to/from the DAX aops vector.

Take the aops write lock while changing DAX state.

We define a new XFS_DAX_EXCL lock type to carry the lock through to transaction completion.

Signed-off-by Ira Weiny <[email protected]>

---
Changes from v3:
Change locking function names to reflect changes in previous patches.

Changes from V2:
Change name of patch (WAS: fs/xfs: Add lock/unlock state to xfs) Remove the xfs specific lock and move to the vfs layer.
We still use XFS_LOCK_DAX_EXCL to be able to pass this flag through to the transaction code. But we no longer have a lock specific to xfs. This removes a lot of code from the XFS layer, preps us for using this in ext4, and is actually more straight forward now that all the locking requirements are better known.

Fix locking order comment
Rework for new 'state' names
(Other comments on the previous patch are not applicable with new patch as much of the code was removed in favor of the vfs level lock)

---
 fs/xfs/xfs_inode.c | 22 ++++++++++++++++++++--
 fs/xfs/xfs_inode.h |  7 +++++--
 2 files changed, 25 insertions(+), 4 deletions(-)

fs/xfs/xfs_inode.c +0 -0

index 35df324875db..5b014c428f0f 100644

fs/xfs/xfs_inode.c +1 -1

  *
  * mmap_sem locking order:
  *
  * i_rwsem -> page lock -> mmap_sem
- * mmap_sem -> i_mmap_lock -> page_lock
+ * s_dax_sem -> mmap_sem -> i_mmap_lock -> page_lock
  *
  * The difference in mmap_sem locking order mean that we cannot hold the
  * i_mmap_lock over syscall based read(2)/write(2) based IO. These IO paths can

fs/xfs/xfs_inode.c +17 -0

+		inode_aops_down_write(VFS_I(ip));
+
 	if (lock_flags & XFS_IOLOCK_EXCL) {
 		down_write_nested(&VFS_I(ip)->i_rwsem,
 				  XFS_IOLOCK_DEP(lock_flags));

@@ -224,6 +227,8 @@ xfs_ilock_nowait(
 	 * You can't set both SHARED and EXCL for the same lock,
 	 * and only XFS_IOLOCK_SHARED, XFS_IOLOCK_EXCL, XFS_ILOCK_SHARED,
 	 * and XFS_ILOCK_EXCL are valid values to set in lock_flags.
+	 *
+	 * XFS_DAX_* is not allowed
 	 */
 	ASSERT((lock_flags & (XFS_IOLOCK_SHARED | XFS_IOLOCK_EXCL)) !=
 	       (XFS_IOLOCK_SHARED | XFS_IOLOCK_EXCL));

@@ -232,6 +237,7 @@ xfs_ilock_nowait(
 	ASSERT((lock_flags & (XFS_ILOCK_SHARED | XFS_ILOCK_EXCL)) !=
 	       (XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
 	ASSERT((lock_flags & ~(XFS_LOCK_MASK | XFS_LOCK_SUBCLASS_MASK)) == 0);
+	ASSERT((lock_flags & XFS_DAX_EXCL) == 0);
 
 	if (lock_flags & XFS_IOLOCK_EXCL) {
 		if (!down_write_trylock(&VFS_I(ip)->i_rwsem))

@@ -318,6 +324,9 @@ xfs_iunlock(
 	else if (lock_flags & XFS_ILOCK_SHARED)
 		mrunlock_shared(&ip->i_lock);
 
+	if (lock_flags & XFS_DAX_EXCL)
+		inode_aops_up_write(VFS_I(ip));
+
 	trace_xfs_iunlock(ip, lock_flags, _RET_IP_);
 }
 

@@ -333,6 +342,8 @@ xfs_ilock_demote(
 	ASSERT(lock_flags & (XFS_IOLOCK_EXCL|XFS_MMAPLOCK_EXCL|XFS_ILOCK_EXCL));
 	ASSERT((lock_flags &
 		~(XFS_IOLOCK_EXCL|XFS_MMAPLOCK_EXCL|XFS_ILOCK_EXCL)) == 0);
+	/* XFS_DAX_* is not allowed */
+	ASSERT((lock_flags & XFS_DAX_EXCL) == 0);
 
 	if (lock_flags & XFS_ILOCK_EXCL)
 		mrdemote(&ip->i_lock);

@@ -465,6 +476,9 @@ xfs_lock_inodes(
 	ASSERT(!(lock_mode & XFS_ILOCK_EXCL) ||
 		inodes <= XFS_ILOCK_MAX_SUBCLASS + 1);
 
+	/* XFS_DAX_* is not allowed */
+	ASSERT((lock_mode & XFS_DAX_EXCL) == 0);
+
 	if (lock_mode & XFS_IOLOCK_EXCL) {
 		ASSERT(!(lock_mode & (XFS_MMAPLOCK_EXCL | XFS_ILOCK_EXCL)));
 	} else if (lock_mode & XFS_MMAPLOCK_EXCL)

@@ -566,6 +580,10 @@ xfs_lock_two_inodes(
 	ASSERT(!(ip0_mode & (XFS_MMAPLOCK_SHARED|XFS_MMAPLOCK_EXCL)) ||
 	       !(ip1_mode & (XFS_ILOCK_SHARED|XFS_ILOCK_EXCL)));
 
+	/* XFS_DAX_* is not allowed */
+	ASSERT((ip0_mode & XFS_DAX_EXCL) == 0);
+	ASSERT((ip1_mode & XFS_DAX_EXCL) == 0);
+
 	ASSERT(ip0->i_ino != ip1->i_ino);
 
 	if (ip0->i_ino > ip1->i_ino) {

fs/xfs/xfs_inode.h +5 -2

index 492e53992fa9..25fe20740bf7 100644
@@ -278,10 +278,12 @@ static inline void xfs_ifunlock(struct xfs_inode *ip)
 #define	XFS_ILOCK_SHARED	(1<<3)
 #define	XFS_MMAPLOCK_EXCL	(1<<4)
 #define	XFS_MMAPLOCK_SHARED	(1<<5)
+#define	XFS_DAX_EXCL		(1<<6)
 
 #define XFS_LOCK_MASK		(XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED \
 				| XFS_ILOCK_EXCL | XFS_ILOCK_SHARED \
-				| XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)
+				| XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED \
+				| XFS_DAX_EXCL)
 
 #define XFS_LOCK_FLAGS \
 	{ XFS_IOLOCK_EXCL,	"IOLOCK_EXCL" }, \

@@ -289,7 +291,8 @@ static inline void xfs_ifunlock(struct xfs_inode *ip)
 	{ XFS_ILOCK_EXCL,	"ILOCK_EXCL" }, \
 	{ XFS_ILOCK_SHARED,	"ILOCK_SHARED" }, \
 	{ XFS_MMAPLOCK_EXCL,	"MMAPLOCK_EXCL" }, \
-	{ XFS_MMAPLOCK_SHARED,	"MMAPLOCK_SHARED" }
+	{ XFS_MMAPLOCK_SHARED,	"MMAPLOCK_SHARED" }, \
+	{ XFS_DAX_EXCL,		"DAX_EXCL" }
 
 
 /*

Signature · 1 line

2.21.0

Quoted 37 lines · On Thu, Feb 20, 2020 at 04:41:30PM -0800, [email protected] wrote:

From: Ira Weiny <[email protected]>

XFS requires the use of the aops of an inode to quiesced prior to changing it to/from the DAX aops vector.

Take the aops write lock while changing DAX state.

We define a new XFS_DAX_EXCL lock type to carry the lock through to transaction completion.

Signed-off-by Ira Weiny <[email protected]>

---
Changes from v3:
Change locking function names to reflect changes in previous patches.

Changes from V2:
Change name of patch (WAS: fs/xfs: Add lock/unlock state to xfs) Remove the xfs specific lock and move to the vfs layer.
We still use XFS_LOCK_DAX_EXCL to be able to pass this flag through to the transaction code. But we no longer have a lock specific to xfs. This removes a lot of code from the XFS layer, preps us for using this in ext4, and is actually more straight forward now that all the locking requirements are better known.

Fix locking order comment
Rework for new 'state' names
(Other comments on the previous patch are not applicable with new patch as much of the code was removed in favor of the vfs level lock)

---
 fs/xfs/xfs_inode.c | 22 ++++++++++++++++++++--
 fs/xfs/xfs_inode.h |  7 +++++--
 2 files changed, 25 insertions(+), 4 deletions(-)

fs/xfs/xfs_inode.c xfs_ilock_attr_map_shared( · line 145

Quoted 10 lines

fs/xfs/xfs_inode.c +1 -1

index 35df324875db..5b014c428f0f 100644
@@ -142,12 +142,12 @@ xfs_ilock_attr_map_shared(
  *
  * Basic locking order:
  *
- * i_rwsem -> i_mmap_lock -> page_lock -> i_ilock
+ * s_dax_sem -> i_rwsem -> i_mmap_lock -> page_lock -> i_ilock

"dax_sem"? I thought this was now called i_aops_sem?

Quoted 14 lines

fs/xfs/xfs_inode.c +1 -1

  *
  * mmap_sem locking order:
  *
  * i_rwsem -> page lock -> mmap_sem
- * mmap_sem -> i_mmap_lock -> page_lock
+ * s_dax_sem -> mmap_sem -> i_mmap_lock -> page_lock
  *
  * The difference in mmap_sem locking order mean that we cannot hold the
  * i_mmap_lock over syscall based read(2)/write(2) based IO. These IO paths can

@@ -182,6 +182,9 @@ xfs_ilock(
 	       (XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
 	ASSERT((lock_flags & ~(XFS_LOCK_MASK | XFS_LOCK_SUBCLASS_MASK)) == 0);
 

fs/xfs/xfs_inode.c xfs_ilock( · line 185

Quoted 1 line

fs/xfs/xfs_inode.c +1 -0

+	if (lock_flags & XFS_DAX_EXCL)

And similarly, I think this should be XFS_OPSLOCK_EXCL...

--D

Quoted 93 lines

fs/xfs/xfs_inode.c +17 -0

+		inode_aops_down_write(VFS_I(ip));
+
 	if (lock_flags & XFS_IOLOCK_EXCL) {
 		down_write_nested(&VFS_I(ip)->i_rwsem,
 				  XFS_IOLOCK_DEP(lock_flags));

@@ -224,6 +227,8 @@ xfs_ilock_nowait(
 	 * You can't set both SHARED and EXCL for the same lock,
 	 * and only XFS_IOLOCK_SHARED, XFS_IOLOCK_EXCL, XFS_ILOCK_SHARED,
 	 * and XFS_ILOCK_EXCL are valid values to set in lock_flags.
+	 *
+	 * XFS_DAX_* is not allowed
 	 */
 	ASSERT((lock_flags & (XFS_IOLOCK_SHARED | XFS_IOLOCK_EXCL)) !=
 	       (XFS_IOLOCK_SHARED | XFS_IOLOCK_EXCL));

@@ -232,6 +237,7 @@ xfs_ilock_nowait(
 	ASSERT((lock_flags & (XFS_ILOCK_SHARED | XFS_ILOCK_EXCL)) !=
 	       (XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
 	ASSERT((lock_flags & ~(XFS_LOCK_MASK | XFS_LOCK_SUBCLASS_MASK)) == 0);
+	ASSERT((lock_flags & XFS_DAX_EXCL) == 0);
 
 	if (lock_flags & XFS_IOLOCK_EXCL) {
 		if (!down_write_trylock(&VFS_I(ip)->i_rwsem))

@@ -318,6 +324,9 @@ xfs_iunlock(
 	else if (lock_flags & XFS_ILOCK_SHARED)
 		mrunlock_shared(&ip->i_lock);
 
+	if (lock_flags & XFS_DAX_EXCL)
+		inode_aops_up_write(VFS_I(ip));
+
 	trace_xfs_iunlock(ip, lock_flags, _RET_IP_);
 }
 

@@ -333,6 +342,8 @@ xfs_ilock_demote(
 	ASSERT(lock_flags & (XFS_IOLOCK_EXCL|XFS_MMAPLOCK_EXCL|XFS_ILOCK_EXCL));
 	ASSERT((lock_flags &
 		~(XFS_IOLOCK_EXCL|XFS_MMAPLOCK_EXCL|XFS_ILOCK_EXCL)) == 0);
+	/* XFS_DAX_* is not allowed */
+	ASSERT((lock_flags & XFS_DAX_EXCL) == 0);
 
 	if (lock_flags & XFS_ILOCK_EXCL)
 		mrdemote(&ip->i_lock);

@@ -465,6 +476,9 @@ xfs_lock_inodes(
 	ASSERT(!(lock_mode & XFS_ILOCK_EXCL) ||
 		inodes <= XFS_ILOCK_MAX_SUBCLASS + 1);
 
+	/* XFS_DAX_* is not allowed */
+	ASSERT((lock_mode & XFS_DAX_EXCL) == 0);
+
 	if (lock_mode & XFS_IOLOCK_EXCL) {
 		ASSERT(!(lock_mode & (XFS_MMAPLOCK_EXCL | XFS_ILOCK_EXCL)));
 	} else if (lock_mode & XFS_MMAPLOCK_EXCL)

@@ -566,6 +580,10 @@ xfs_lock_two_inodes(
 	ASSERT(!(ip0_mode & (XFS_MMAPLOCK_SHARED|XFS_MMAPLOCK_EXCL)) ||
 	       !(ip1_mode & (XFS_ILOCK_SHARED|XFS_ILOCK_EXCL)));
 
+	/* XFS_DAX_* is not allowed */
+	ASSERT((ip0_mode & XFS_DAX_EXCL) == 0);
+	ASSERT((ip1_mode & XFS_DAX_EXCL) == 0);
+
 	ASSERT(ip0->i_ino != ip1->i_ino);
 
 	if (ip0->i_ino > ip1->i_ino) {

fs/xfs/xfs_inode.h +5 -2

index 492e53992fa9..25fe20740bf7 100644
@@ -278,10 +278,12 @@ static inline void xfs_ifunlock(struct xfs_inode *ip)
 #define	XFS_ILOCK_SHARED	(1<<3)
 #define	XFS_MMAPLOCK_EXCL	(1<<4)
 #define	XFS_MMAPLOCK_SHARED	(1<<5)
+#define	XFS_DAX_EXCL		(1<<6)
 
 #define XFS_LOCK_MASK		(XFS_IOLOCK_EXCL | XFS_IOLOCK_SHARED \
 				| XFS_ILOCK_EXCL | XFS_ILOCK_SHARED \
-				| XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED)
+				| XFS_MMAPLOCK_EXCL | XFS_MMAPLOCK_SHARED \
+				| XFS_DAX_EXCL)
 
 #define XFS_LOCK_FLAGS \
 	{ XFS_IOLOCK_EXCL,	"IOLOCK_EXCL" }, \

@@ -289,7 +291,8 @@ static inline void xfs_ifunlock(struct xfs_inode *ip)
 	{ XFS_ILOCK_EXCL,	"ILOCK_EXCL" }, \
 	{ XFS_ILOCK_SHARED,	"ILOCK_SHARED" }, \
 	{ XFS_MMAPLOCK_EXCL,	"MMAPLOCK_EXCL" }, \
-	{ XFS_MMAPLOCK_SHARED,	"MMAPLOCK_SHARED" }
+	{ XFS_MMAPLOCK_SHARED,	"MMAPLOCK_SHARED" }, \
+	{ XFS_DAX_EXCL,		"DAX_EXCL" }
 
 
 /*

Signature · 1 line

2.21.0

From: Ira Weiny <[email protected]>

XFS requires the use of the aops of an inode to quiesced prior to changing it to/from the DAX aops vector.

Take the aops write lock while changing DAX state.

We define a new XFS_DAX_EXCL lock type to carry the lock through to transaction completion.

Signed-off-by Ira Weiny <[email protected]>

---
Changes from v3:
Change locking function names to reflect changes in previous patches.

Changes from V2:
Change name of patch (WAS: fs/xfs: Add lock/unlock state to xfs) Remove the xfs specific lock and move to the vfs layer.
We still use XFS_LOCK_DAX_EXCL to be able to pass this flag through to the transaction code. But we no longer have a lock specific to xfs. This removes a lot of code from the XFS layer, preps us for using this in ext4, and is actually more straight forward now that all the locking requirements are better known.

Fix locking order comment
Rework for new 'state' names
(Other comments on the previous patch are not applicable with new patch as much of the code was removed in favor of the vfs level lock)

---
 fs/xfs/xfs_inode.c | 22 ++++++++++++++++++++--
 fs/xfs/xfs_inode.h |  7 +++++--
 2 files changed, 25 insertions(+), 4 deletions(-)

fs/xfs/xfs_inode.c +2 -2

index 35df324875db..5b014c428f0f 100644
@@ -142,12 +142,12 @@ xfs_ilock_attr_map_shared(
  *
  * Basic locking order:
  *
- * i_rwsem -> i_mmap_lock -> page_lock -> i_ilock
+ * s_dax_sem -> i_rwsem -> i_mmap_lock -> page_lock -> i_ilock
  *
  * mmap_sem locking order:
  *
  * i_rwsem -> page lock -> mmap_sem
- * mmap_sem -> i_mmap_lock -> page_lock
+ * s_dax_sem -> mmap_sem -> i_mmap_lock -> page_lock
  *
  * The difference in mmap_sem locking order mean that we cannot hold the
  * i_mmap_lock over syscall based read(2)/write(2) based IO. These IO paths can

Quoted 2 lines · On Thu, Feb 20, 2020 at 04:41:30PM -0800, [email protected] wrote:

From: Ira Weiny <[email protected]>

Quoted 21 lines

fs/xfs/xfs_inode.c +2 -2

index 35df324875db..5b014c428f0f 100644
@@ -142,12 +142,12 @@ xfs_ilock_attr_map_shared(
  *
  * Basic locking order:
  *
- * i_rwsem -> i_mmap_lock -> page_lock -> i_ilock
+ * s_dax_sem -> i_rwsem -> i_mmap_lock -> page_lock -> i_ilock
  *
  * mmap_sem locking order:
  *
  * i_rwsem -> page lock -> mmap_sem
- * mmap_sem -> i_mmap_lock -> page_lock
+ * s_dax_sem -> mmap_sem -> i_mmap_lock -> page_lock
  *
  * The difference in mmap_sem locking order mean that we cannot hold the
  * i_mmap_lock over syscall based read(2)/write(2) based IO. These IO paths can

fs/xfs/xfs_inode.c xfs_ilock( · line 186

Quoted 5 lines

fs/xfs/xfs_inode.c +2 -0

@@ -182,6 +182,9 @@ xfs_ilock(
 	       (XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
 	ASSERT((lock_flags & ~(XFS_LOCK_MASK | XFS_LOCK_SUBCLASS_MASK)) == 0);
 
+	if (lock_flags & XFS_DAX_EXCL)
+		inode_aops_down_write(VFS_I(ip));

I largely don't see the point of adding this to xfs_ilock/iunlock.

It's only got one caller, so I don't see much point in adding it to an interface that has over a hundred other call sites that don't need or use this lock. just open code it where it is needed in the ioctl code.

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Quoted 3 lines · Dave Chinner

Quoted 2 lines · On Thu, Feb 20, 2020 at 04:41:30PM -0800, [email protected] wrote:

From: Ira Weiny <[email protected]>

[snip]

Quoted 29 lines

Quoted 21 lines

fs/xfs/xfs_inode.c +2 -2

index 35df324875db..5b014c428f0f 100644
@@ -142,12 +142,12 @@ xfs_ilock_attr_map_shared(
  *
  * Basic locking order:
  *
- * i_rwsem -> i_mmap_lock -> page_lock -> i_ilock
+ * s_dax_sem -> i_rwsem -> i_mmap_lock -> page_lock -> i_ilock
  *
  * mmap_sem locking order:
  *
  * i_rwsem -> page lock -> mmap_sem
- * mmap_sem -> i_mmap_lock -> page_lock
+ * s_dax_sem -> mmap_sem -> i_mmap_lock -> page_lock
  *
  * The difference in mmap_sem locking order mean that we cannot hold the
  * i_mmap_lock over syscall based read(2)/write(2) based IO. These IO paths can

fs/xfs/xfs_inode.c xfs_ilock( · line 186

Quoted 5 lines

fs/xfs/xfs_inode.c +2 -0

@@ -182,6 +182,9 @@ xfs_ilock(
 	       (XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
 	ASSERT((lock_flags & ~(XFS_LOCK_MASK | XFS_LOCK_SUBCLASS_MASK)) == 0);
 
+	if (lock_flags & XFS_DAX_EXCL)
+		inode_aops_down_write(VFS_I(ip));

I largely don't see the point of adding this to xfs_ilock/iunlock.

Quoted 4 lines

It's only got one caller, so I don't see much point in adding it to an interface that has over a hundred other call sites that don't need or use this lock. just open code it where it is needed in the ioctl code.

I know it seems overkill but if we don't do this we need to code a flag to be returned from xfs_ioctl_setattr_dax_invalidate(). This flag is then used in xfs_ioctl_setattr_get_trans() to create the transaction log item which can then be properly used to unlock the lock in xfs_inode_item_release()

I don't know of a cleaner way to communicate to xfs_inode_item_release() to unlock i_aops_sem after the transaction is complete.

Quoted 41 lines · Ira Weiny

Quoted 3 lines · Dave Chinner

Quoted 2 lines · On Thu, Feb 20, 2020 at 04:41:30PM -0800, [email protected] wrote:

From: Ira Weiny <[email protected]>

[snip]

Quoted 29 lines

Quoted 21 lines

fs/xfs/xfs_inode.c +2 -2

index 35df324875db..5b014c428f0f 100644
@@ -142,12 +142,12 @@ xfs_ilock_attr_map_shared(
  *
  * Basic locking order:
  *
- * i_rwsem -> i_mmap_lock -> page_lock -> i_ilock
+ * s_dax_sem -> i_rwsem -> i_mmap_lock -> page_lock -> i_ilock
  *
  * mmap_sem locking order:
  *
  * i_rwsem -> page lock -> mmap_sem
- * mmap_sem -> i_mmap_lock -> page_lock
+ * s_dax_sem -> mmap_sem -> i_mmap_lock -> page_lock
  *
  * The difference in mmap_sem locking order mean that we cannot hold the
  * i_mmap_lock over syscall based read(2)/write(2) based IO. These IO paths can

fs/xfs/xfs_inode.c xfs_ilock( · line 186

Quoted 5 lines

fs/xfs/xfs_inode.c +2 -0

@@ -182,6 +182,9 @@ xfs_ilock(
 	       (XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
 	ASSERT((lock_flags & ~(XFS_LOCK_MASK | XFS_LOCK_SUBCLASS_MASK)) == 0);
 
+	if (lock_flags & XFS_DAX_EXCL)
+		inode_aops_down_write(VFS_I(ip));

I largely don't see the point of adding this to xfs_ilock/iunlock.

Quoted 4 lines

It's only got one caller, so I don't see much point in adding it to an interface that has over a hundred other call sites that don't need or use this lock. just open code it where it is needed in the ioctl code.

Quoted 7 lines

I know it seems overkill but if we don't do this we need to code a flag to be returned from xfs_ioctl_setattr_dax_invalidate(). This flag is then used in xfs_ioctl_setattr_get_trans() to create the transaction log item which can then be properly used to unlock the lock in xfs_inode_item_release()

I don't know of a cleaner way to communicate to xfs_inode_item_release() to unlock i_aops_sem after the transaction is complete.

We manually unlock inodes after transactions in many cases - anywhere we do a rolling transaction, the inode locks do not get released by the transaction. Hence for a one-off case like this it doesn't really make sense to push all this infrastructure into the transaction subsystem. Especially as we can manually lock before and unlock after the transaction context without any real complexity.

This also means that we can, if necessary, do aops manipulation work /after/ the transaction that changes on-disk state completes and we still hold the aops reference exclusively. While we don't do that now, I think it is worthwhile keeping our options open here....

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Quoted 50 lines · Dave Chinner

Quoted 48 lines · Ira Weiny

Quoted 3 lines · Dave Chinner

Quoted 2 lines · On Thu, Feb 20, 2020 at 04:41:30PM -0800, [email protected] wrote:

From: Ira Weiny <[email protected]>

[snip]

Quoted 29 lines

Quoted 21 lines

fs/xfs/xfs_inode.c +2 -2

index 35df324875db..5b014c428f0f 100644
@@ -142,12 +142,12 @@ xfs_ilock_attr_map_shared(
  *
  * Basic locking order:
  *
- * i_rwsem -> i_mmap_lock -> page_lock -> i_ilock
+ * s_dax_sem -> i_rwsem -> i_mmap_lock -> page_lock -> i_ilock
  *
  * mmap_sem locking order:
  *
  * i_rwsem -> page lock -> mmap_sem
- * mmap_sem -> i_mmap_lock -> page_lock
+ * s_dax_sem -> mmap_sem -> i_mmap_lock -> page_lock
  *
  * The difference in mmap_sem locking order mean that we cannot hold the
  * i_mmap_lock over syscall based read(2)/write(2) based IO. These IO paths can

fs/xfs/xfs_inode.c xfs_ilock( · line 186

Quoted 5 lines

fs/xfs/xfs_inode.c +2 -0

@@ -182,6 +182,9 @@ xfs_ilock(
 	       (XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
 	ASSERT((lock_flags & ~(XFS_LOCK_MASK | XFS_LOCK_SUBCLASS_MASK)) == 0);
 
+	if (lock_flags & XFS_DAX_EXCL)
+		inode_aops_down_write(VFS_I(ip));

I largely don't see the point of adding this to xfs_ilock/iunlock.

Quoted 4 lines

It's only got one caller, so I don't see much point in adding it to an interface that has over a hundred other call sites that don't need or use this lock. just open code it where it is needed in the ioctl code.

I know it seems overkill but if we don't do this we need to code a flag to be returned from xfs_ioctl_setattr_dax_invalidate(). This flag is then used in xfs_ioctl_setattr_get_trans() to create the transaction log item which can then be properly used to unlock the lock in xfs_inode_item_release()

I don't know of a cleaner way to communicate to xfs_inode_item_release() to unlock i_aops_sem after the transaction is complete.

Quoted 6 lines

We manually unlock inodes after transactions in many cases - anywhere we do a rolling transaction, the inode locks do not get released by the transaction. Hence for a one-off case like this it doesn't really make sense to push all this infrastructure into the transaction subsystem. Especially as we can manually lock before and unlock after the transaction context without any real complexity.

So does xfs_trans_commit() operate synchronously?

I want to understand this better because I have fought with a lot of ABBA issues with these locks. So... can I hold the lock until after xfs_trans_commit() and safely unlock it there... because the XFS_MMAPLOCK_EXCL, XFS_IOLOCK_EXCL, and XFS_ILOCK_EXCL will be released at that point? Thus preserving the following lock order.

Quoted 51 lines · Ira Weiny

Quoted 50 lines · Dave Chinner

Quoted 48 lines · Ira Weiny

Quoted 3 lines · Dave Chinner

Quoted 2 lines · On Thu, Feb 20, 2020 at 04:41:30PM -0800, [email protected] wrote:

From: Ira Weiny <[email protected]>

[snip]

Quoted 29 lines

Quoted 21 lines

fs/xfs/xfs_inode.c +2 -2

index 35df324875db..5b014c428f0f 100644
@@ -142,12 +142,12 @@ xfs_ilock_attr_map_shared(
  *
  * Basic locking order:
  *
- * i_rwsem -> i_mmap_lock -> page_lock -> i_ilock
+ * s_dax_sem -> i_rwsem -> i_mmap_lock -> page_lock -> i_ilock
  *
  * mmap_sem locking order:
  *
  * i_rwsem -> page lock -> mmap_sem
- * mmap_sem -> i_mmap_lock -> page_lock
+ * s_dax_sem -> mmap_sem -> i_mmap_lock -> page_lock
  *
  * The difference in mmap_sem locking order mean that we cannot hold the
  * i_mmap_lock over syscall based read(2)/write(2) based IO. These IO paths can

fs/xfs/xfs_inode.c xfs_ilock( · line 186

Quoted 5 lines

fs/xfs/xfs_inode.c +2 -0

@@ -182,6 +182,9 @@ xfs_ilock(
 	       (XFS_ILOCK_SHARED | XFS_ILOCK_EXCL));
 	ASSERT((lock_flags & ~(XFS_LOCK_MASK | XFS_LOCK_SUBCLASS_MASK)) == 0);
 
+	if (lock_flags & XFS_DAX_EXCL)
+		inode_aops_down_write(VFS_I(ip));

I largely don't see the point of adding this to xfs_ilock/iunlock.

Quoted 4 lines

It's only got one caller, so I don't see much point in adding it to an interface that has over a hundred other call sites that don't need or use this lock. just open code it where it is needed in the ioctl code.

I know it seems overkill but if we don't do this we need to code a flag to be returned from xfs_ioctl_setattr_dax_invalidate(). This flag is then used in xfs_ioctl_setattr_get_trans() to create the transaction log item which can then be properly used to unlock the lock in xfs_inode_item_release()

I don't know of a cleaner way to communicate to xfs_inode_item_release() to unlock i_aops_sem after the transaction is complete.

Quoted 8 lines

Quoted 6 lines

We manually unlock inodes after transactions in many cases - anywhere we do a rolling transaction, the inode locks do not get released by the transaction. Hence for a one-off case like this it doesn't really make sense to push all this infrastructure into the transaction subsystem. Especially as we can manually lock before and unlock after the transaction context without any real complexity.

So does xfs_trans_commit() operate synchronously?

What do you mean by "synchronously", and what are you expecting to occur (a)synchronously with respect to filesystem objects and/or on-disk state?

Keep in mid that the xfs transaction subsystem is a complex asynchronous IO engine full of feedback loops and resource management,

so asking if something is "synchronous" without any other context is a difficult question to answer :)

Quoted 6 lines

I want to understand this better because I have fought with a lot of ABBA issues with these locks. So... can I hold the lock until after xfs_trans_commit() and safely unlock it there... because the XFS_MMAPLOCK_EXCL, XFS_IOLOCK_EXCL, and XFS_ILOCK_EXCL will be released at that point? Thus preserving the following lock order.

See how operations like xfs_create, xfs_unlink, etc work. The don't specify flags to xfs_ijoin(), and so the transaction commits don't automatically unlock the inode.

This is necessary so that rolling
transactions are executed atomically w.r.t. inode access - no-one can lock and access the inode while a multi-commit rolling transaction on the inode is on-going.

In this case it's just a single commit and we don't need to keep it locked after the change is made, so we can unlock the inode on commit. So for the XFS internal locks the code is fine and doesn't need to change. We just need to wrap the VFS aops lock (if we keep it) around the outside of all the XFS locking until the transaction commits and unlocks the XFS locks...

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

From: Ira Weiny <[email protected]>

Define a variable to hold the lock flags to ensure that the correct locks are returned or released on error.

Quoted 4 lines · On Thu, Feb 20, 2020 at 04:41:31PM -0800, [email protected] wrote:

From: Ira Weiny <[email protected]>

Define a variable to hold the lock flags to ensure that the correct locks are returned or released on error.

I don't see how this cleans up anything..