Multi-CPU harmless lockdep on x86 while copying data

Hi! I was playing a shell game with some precious backup data. It went like this:

open a 36-GB source partition (DM linear, partitions from 2 drives,
v5-superblock XFS)

open a 32-GB aes-xts crypt (v4-superblock XFS)

cp -av from holding partition to crypt

It was during the cp operation that I got this multi-CPU version of the harmless lockdep:

=========================================================
[ INFO: possible irq lock inversion dependency detected ] 3.14.0-rc5+ #6 Not tainted
---------------------------------------------------------
kswapd0/25 just changed the state of lock:
(&xfs_dir_ilock_class){++++-.}, at: [<791c09fa>] xfs_ilock+0xff/0x16c
but this lock took another, RECLAIM_FS-unsafe lock in the past:
(&mm->mmap_sem){++++++}

and interrupts could create inverse lock ordering between them.

other info that might help us debug this:
Possible interrupt unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(&mm->mmap_sem);
                               local_irq_disable();
                               lock(&xfs_dir_ilock_class);
                               lock(&mm->mmap_sem);
  <Interrupt>
    lock(&xfs_dir_ilock_class);

  }
  ... key      at: [<795f1eec>] __key.44037+0x0/0x8
  ... acquired at:
   [<79064360>] __lock_acquire+0x397/0xa6c
   [<7906510f>] lock_acquire+0x8b/0x101

   [<790d1718>] might_fault+0x81/0xa9
   [<790f379a>] filldir64+0x92/0xe2
   [<7916f079>] xfs_dir2_sf_getdents+0x1a0/0x44c
   [<7917009e>] xfs_readdir+0xc4/0x126
   [<79171b8b>] xfs_file_readdir+0x25/0x3e
   [<790f385a>] iterate_dir+0x70/0x9b
   [<790f3a31>] SyS_getdents64+0x6d/0xcc
   [<792f85b8>] sysenter_do_call+0x12/0x36

I call it harmless because its single-CPU variant can be reproduced as far back as I could bisect in earlier testing (way before kernel 2.6.20). However, such lockdep splats have never manifested in a noticeable problem on production kernels on x86. Either down_write_nested or down_read_nested is in all of them, depending on which kernel version is in use. At least one reclaim-related function is in there as well. vm_map_ram used to be in there, but Dave took care of that (thanks!).

This particular splat has been showing up more often, though. It's not tied to one particular commit, event, or change; just something that has crept in gradually since maybe kernel 3.11. It's like watching grass grow or watching paint dry.

Changing the directory code to handle this sort of locking is going to require a bit of surgery. However, I can see advantages to moving directory data to the same locking strategy as regular file data - locking heirarchies are identical, directory ilock hold times are much reduced, we don't get lockdep whining about taking page faults with the ilock held, etc.

A quick hack at to demonstrate the high level, initial step of using the IOLOCK for readdir serialisation. I've done a little smoke testing on it, so it won't die immediately. It should get rid of all the nasty lockdep issues, but it doesn't start to address the deeper restructing that is needed.

This would be a straight forward change, except for two things:
filestreams and lockdep. The filestream allocator takes the directory iolock and makes assumptions about parent->child locking order of the iolock which will now be invalidated. Hence some changes to the filestreams code is needed to ensure that it never blocks on directory iolocks and deadlocks. instead it needs to fail stream associations when such problems occur.

From looking over some of the filestreams code just for a few minutes

I think the right fix is to stop abusing the iolock in filestreams. To me it seems like a look inside fstrm_item_t should be fine for what the filestreams code wants if I understand it correctly.

Quoted 3 lines · Christoph Hellwig

I think the right fix is to stop abusing the iolock in filestreams. To me it seems like a look inside fstrm_item_t should be fine for what the filestreams code wants if I understand it correctly.

Seems like the iolock could be removed fairly easily by using either of the two options:

a) reference count fstrm_item, and just grab a reference to it for each child as well as the parent and insert it multiple times. Kill ->pip.
b) only allocate and insert fstrm_items for directories. Find the directory by grabbing an entry off inode->i_dentry and then grabbing the parent. There always should be a dentry around when we allocate blocks, and if none we can just skip out of the filestreams allocator if there's none. For the cases that matter there is.

Both mean that the race it tries to protect against using the iolock is remove entirely, and the code becomes more efficient as well. Option a) seems simple to implement, but b) will save a lot more memory and operations when using the filestreams allocator.

Yeah, b) seems like the way to simplify it - the filestreams code really only needs to track the parent/ag relationship rather than the child/parent relationship if there is a reliable way of determining the parent from the child.

What do we do with hardlinked files in this case? I'm happy to say "too bad" for these files mainly because the filestream allocator is aimed at associating multiple file creations together, so hard links really don't matter AFAICT...

Quoted 7 lines · Dave Chinner

Changing the directory code to handle this sort of locking is going to require a bit of surgery. However, I can see advantages to moving directory data to the same locking strategy as regular file data - locking heirarchies are identical, directory ilock hold times are much reduced, we don't get lockdep whining about taking page faults with the ilock held, etc.

Quoted 5 lines

A quick hack at to demonstrate the high level, initial step of using the IOLOCK for readdir serialisation. I've done a little smoke testing on it, so it won't die immediately. It should get rid of all the nasty lockdep issues, but it doesn't start to address the deeper restructing that is needed.

What synchronization do we actually need from the iolock? Pushing the ilock down to where it's actually needed is a good idea either way, though.

Quoted 7 lines

This would be a straight forward change, except for two things:
filestreams and lockdep. The filestream allocator takes the directory iolock and makes assumptions about parent->child locking order of the iolock which will now be invalidated. Hence some changes to the filestreams code is needed to ensure that it never blocks on directory iolocks and deadlocks. instead it needs to fail stream associations when such problems occur.

I think the right fix is to stop abusing the iolock in filestreams. To me it seems like a look inside fstrm_item_t should be fine for what the filestreams code wants if I understand it correctly.

From looking over some of the filestreams code just for a few minutes I get an urge to redo lots of it right now..

fs/xfs/xfs_inode.c xfs_create( · line 1247

Quoted 6 lines

fs/xfs/xfs_inode.c +1 -1

@@ -1228,7 +1244,7 @@ xfs_create(
 	 * the transaction cancel unlocking dp so don't do it explicitly in the
 	 * error path.
 	 */
-	xfs_trans_ijoin(tp, dp, XFS_ILOCK_EXCL);
+	xfs_trans_ijoin(tp, dp, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL);

What do we need the iolock on these operations for?

Quoted 14 lines · Christoph Hellwig

Quoted 7 lines · Dave Chinner

Changing the directory code to handle this sort of locking is going to require a bit of surgery. However, I can see advantages to moving directory data to the same locking strategy as regular file data - locking heirarchies are identical, directory ilock hold times are much reduced, we don't get lockdep whining about taking page faults with the ilock held, etc.

Quoted 5 lines

A quick hack at to demonstrate the high level, initial step of using the IOLOCK for readdir serialisation. I've done a little smoke testing on it, so it won't die immediately. It should get rid of all the nasty lockdep issues, but it doesn't start to address the deeper restructing that is needed.

Quoted 3 lines

What synchronization do we actually need from the iolock? Pushing the ilock down to where it's actually needed is a good idea either way, though.

The issue is that if we push the ilock down to the just the block mapping routines, the directory can be modified while the readdir is in progress. That's the root problem that adding the ilock solved. Now, just pushing the ilock down to protect the bmbt lookups might result in a consistent lookup, but it won't serialise sanely against modifications.

i.e. readdir only walks one dir block at a time but
it maps multiple blocks for readahead and keeps them in a local array and doesn't validate them again before issuing read o nthose buffers. Hence at a high level we currently have to serialise readdir against all directory modifications.

The only other option we might have is to completely rewrite the directory readahead code not to cache mappings. If we use the ilock purely for bmbt lookup and buffer read, then the ilock will serialise against modification, and the buffer lock will stabilise the buffer until the readdir moves to the next buffer and picks the ilock up again to read it.

That would avoid the need for high level serialisation, but it's a lot more work than using the iolock to provide the high level serialisation and i'm still not sure it's 100% safe. And I've got no idea if it would work for CXFS. Hopefully someone from SGI will chime in here....

Quoted 8 lines

Quoted 7 lines

This would be a straight forward change, except for two things:
filestreams and lockdep. The filestream allocator takes the directory iolock and makes assumptions about parent->child locking order of the iolock which will now be invalidated. Hence some changes to the filestreams code is needed to ensure that it never blocks on directory iolocks and deadlocks. instead it needs to fail stream associations when such problems occur.

Quoted 6 lines

I think the right fix is to stop abusing the iolock in filestreams. To me it seems like a look inside fstrm_item_t should be fine for what the filestreams code wants if I understand it correctly.

From looking over some of the filestreams code just for a few minutes I get an urge to redo lots of it right now..

I get that urge from time to time, too. So far I've managed to avoid it.

Quoted 8 lines

fs/xfs/xfs_inode.c xfs_create( · line 1247

Quoted 6 lines

fs/xfs/xfs_inode.c +1 -1

@@ -1228,7 +1244,7 @@ xfs_create(
 	 * the transaction cancel unlocking dp so don't do it explicitly in the
 	 * error path.
 	 */
-	xfs_trans_ijoin(tp, dp, XFS_ILOCK_EXCL);
+	xfs_trans_ijoin(tp, dp, XFS_IOLOCK_EXCL | XFS_ILOCK_EXCL);

What do we need the iolock on these operations for?

These are providing the high level readdir vs modification serialisation protection. And we have to unlock it on transaction commit, which is why it needs to be added to the xfs_trans_ijoin() calls...

Hi,

Quoted 39 lines · Dave Chinner

Quoted 14 lines · Christoph Hellwig

Quoted 7 lines · Dave Chinner

Changing the directory code to handle this sort of locking is going to require a bit of surgery. However, I can see advantages to moving directory data to the same locking strategy as regular file data - locking heirarchies are identical, directory ilock hold times are much reduced, we don't get lockdep whining about taking page faults with the ilock held, etc.

Quoted 5 lines

A quick hack at to demonstrate the high level, initial step of using the IOLOCK for readdir serialisation. I've done a little smoke testing on it, so it won't die immediately. It should get rid of all the nasty lockdep issues, but it doesn't start to address the deeper restructing that is needed.

Quoted 3 lines

What synchronization do we actually need from the iolock? Pushing the ilock down to where it's actually needed is a good idea either way, though.

The issue is that if we push the ilock down to the just the block mapping routines, the directory can be modified while the readdir is in progress. That's the root problem that adding the ilock solved. Now, just pushing the ilock down to protect the bmbt lookups might result in a consistent lookup, but it won't serialise sanely against modifications.

i.e. readdir only walks one dir block at a time but
it maps multiple blocks for readahead and keeps them in a local array and doesn't validate them again before issuing read o nthose buffers. Hence at a high level we currently have to serialise readdir against all directory modifications.

The only other option we might have is to completely rewrite the directory readahead code not to cache mappings. If we use the ilock purely for bmbt lookup and buffer read, then the ilock will serialise against modification, and the buffer lock will stabilise the buffer until the readdir moves to the next buffer and picks the ilock up again to read it.

Quoted 5 lines

That would avoid the need for high level serialisation, but it's a lot more work than using the iolock to provide the high level serialisation and i'm still not sure it's 100% safe. And I've got no idea if it would work for CXFS. Hopefully someone from SGI will chime in here....

Also in leaf and node formats a single modification can change multiple buffers, so I suspect the buffer lock isn't enough serialization to maintain a consistent directory in the face of multiple readers and writers. The iolock does resolve that issue.

Quoted 48 lines · Ben Myers

Hi,

Quoted 39 lines · Dave Chinner

Quoted 14 lines · Christoph Hellwig

Quoted 7 lines · Dave Chinner

Changing the directory code to handle this sort of locking is going to require a bit of surgery. However, I can see advantages to moving directory data to the same locking strategy as regular file data - locking heirarchies are identical, directory ilock hold times are much reduced, we don't get lockdep whining about taking page faults with the ilock held, etc.

Quoted 5 lines

A quick hack at to demonstrate the high level, initial step of using the IOLOCK for readdir serialisation. I've done a little smoke testing on it, so it won't die immediately. It should get rid of all the nasty lockdep issues, but it doesn't start to address the deeper restructing that is needed.

Quoted 3 lines

What synchronization do we actually need from the iolock? Pushing the ilock down to where it's actually needed is a good idea either way, though.

The issue is that if we push the ilock down to the just the block mapping routines, the directory can be modified while the readdir is in progress. That's the root problem that adding the ilock solved. Now, just pushing the ilock down to protect the bmbt lookups might result in a consistent lookup, but it won't serialise sanely against modifications.

i.e. readdir only walks one dir block at a time but
it maps multiple blocks for readahead and keeps them in a local array and doesn't validate them again before issuing read o nthose buffers. Hence at a high level we currently have to serialise readdir against all directory modifications.

The only other option we might have is to completely rewrite the directory readahead code not to cache mappings. If we use the ilock purely for bmbt lookup and buffer read, then the ilock will serialise against modification, and the buffer lock will stabilise the buffer until the readdir moves to the next buffer and picks the ilock up again to read it.

Quoted 5 lines

That would avoid the need for high level serialisation, but it's a lot more work than using the iolock to provide the high level serialisation and i'm still not sure it's 100% safe. And I've got no idea if it would work for CXFS. Hopefully someone from SGI will chime in here....

Quoted 4 lines

Also in leaf and node formats a single modification can change multiple buffers, so I suspect the buffer lock isn't enough serialization to maintain a consistent directory in the face of multiple readers and writers. The iolock does resolve that issue.

Right, but we don't care about anything other than the leaf block that we are currently reading is consistent when the read starts and is consistent across the entire processing. i.e. if the leaf is locked by readdir, then the modification is completely stalled until the readdir lets it go. And readdir then can't get the next buffer until the modification is complete because it blocks on the ilock to get the next mapping and buffer....

Quoted 28 lines · Michael L. Semon

Hi! I was playing a shell game with some precious backup data. It went like this:

open a 36-GB source partition (DM linear, partitions from 2 drives,
v5-superblock XFS)

open a 32-GB aes-xts crypt (v4-superblock XFS)

cp -av from holding partition to crypt

It was during the cp operation that I got this multi-CPU version of the harmless lockdep:

=========================================================
[ INFO: possible irq lock inversion dependency detected ] 3.14.0-rc5+ #6 Not tainted
---------------------------------------------------------
kswapd0/25 just changed the state of lock:
(&xfs_dir_ilock_class){++++-.}, at: [<791c09fa>] xfs_ilock+0xff/0x16c
but this lock took another, RECLAIM_FS-unsafe lock in the past:
(&mm->mmap_sem){++++++}

and interrupts could create inverse lock ordering between them.

other info that might help us debug this:
Possible interrupt unsafe locking scenario:

Quoted 8 lines

       CPU0                    CPU1
       ----                    ----
  lock(&mm->mmap_sem);
                               local_irq_disable();
                               lock(&xfs_dir_ilock_class);
                               lock(&mm->mmap_sem);
  <Interrupt>
    lock(&xfs_dir_ilock_class);

Well, that's rather interesting. I'd love to know where we take the directory ilock with interupts disabled - and then take a page fault... :/

....

Quoted 5 lines

  }
  ... key      at: [<795f1eec>] __key.44037+0x0/0x8
  ... acquired at:
   [<79064360>] __lock_acquire+0x397/0xa6c
   [<7906510f>] lock_acquire+0x8b/0x101

Quoted 8 lines

   [<790d1718>] might_fault+0x81/0xa9
   [<790f379a>] filldir64+0x92/0xe2
   [<7916f079>] xfs_dir2_sf_getdents+0x1a0/0x44c
   [<7917009e>] xfs_readdir+0xc4/0x126
   [<79171b8b>] xfs_file_readdir+0x25/0x3e
   [<790f385a>] iterate_dir+0x70/0x9b
   [<790f3a31>] SyS_getdents64+0x6d/0xcc
   [<792f85b8>] sysenter_do_call+0x12/0x36

All of these paths are from the syscall path, except for one in kswapd context. I don't see any stack trace here that could result in the above "deadlock". It looks to be yet another false positive...

Quoted 9 lines

I call it harmless because its single-CPU variant can be reproduced as far back as I could bisect in earlier testing (way before kernel 2.6.20). However, such lockdep splats have never manifested in a noticeable problem on production kernels on x86. Either down_write_nested or down_read_nested is in all of them, depending on which kernel version is in use. At least one reclaim-related function is in there as well. vm_map_ram used to be in there, but Dave took care of that (thanks!).

Quoted 4 lines

This particular splat has been showing up more often, though. It's not tied to one particular commit, event, or change; just something that has crept in gradually since maybe kernel 3.11. It's like watching grass grow or watching paint dry.

The above reports all indicate that the taking a page fault while holding the directory ilock is problematic. So have all the other new lockdep reports we've got that have been caused by that change.

Realistically, I think that the readdir locking change was fundamentally broken. Why? Because taking page faults while holding the ilock violates the lock order we have for inodes. For regular files, the lock heirarchy is iolock -> page fault -> ilock, and we got rid of the need for the iolock in the inode reclaim path because of all the problems it caused lockdep. Now we've gone and reintroduced that same set of problems - only worse - by allowing the readdir code to take page faults while holding the ilock....

I'd like to revert the change that introduced the ilock into the readdir path, but I suspect that woul dbe an unpopular direction to take. However, we need a solution that doesn't drive us insane with lockdep false positives.

IOWs, we need to do this differently - readdir needs to be protected by the iolock, not the ilock, and only the block mapping routines in the readdir code should be protected by the ilock. i.e. the same locking strategy that is used for regular files: iolock protects the contents of the file, the ilock protects the inode metadata.

What I think we really need to do is drive the ilock all the way into the block mapping functions of the directory code and to the places where the inode metadata is actually going to be modified. The iolock protects access to the directory data, and logging of changes to those blocks is serialised by the buffer locks, so we don't actually need the inode ilock to serialise access to the directory data - we only need the ilock for modifications to the inode and it's blockmaps itself, same as for regular files.

Changing the directory code to handle this sort of locking is going to require a bit of surgery. However, I can see advantages to moving directory data to the same locking strategy as regular file data - locking heirarchies are identical, directory ilock hold times are much reduced, we don't get lockdep whining about taking page faults with the ilock held, etc.

A quick hack at to demonstrate the high level, initial step of using the IOLOCK for readdir serialisation. I've done a little smoke testing on it, so it won't die immediately. It should get rid of all the nasty lockdep issues, but it doesn't start to address the deeper restructing that is needed.

Thoughts, comments? I'm especially interested in what SGI people have to say here because this locking change was needed for CXFS, and changing the directory locking iheirarchy is probably going to upset CXFS a lot...

Hi Dave,

Quoted 41 lines · Dave Chinner

Quoted 28 lines · Michael L. Semon

Hi! I was playing a shell game with some precious backup data. It went like this:

open a 36-GB source partition (DM linear, partitions from 2 drives,
v5-superblock XFS)

open a 32-GB aes-xts crypt (v4-superblock XFS)

cp -av from holding partition to crypt

It was during the cp operation that I got this multi-CPU version of the harmless lockdep:

=========================================================
[ INFO: possible irq lock inversion dependency detected ] 3.14.0-rc5+ #6 Not tainted
---------------------------------------------------------
kswapd0/25 just changed the state of lock:
(&xfs_dir_ilock_class){++++-.}, at: [<791c09fa>] xfs_ilock+0xff/0x16c
but this lock took another, RECLAIM_FS-unsafe lock in the past:
(&mm->mmap_sem){++++++}

and interrupts could create inverse lock ordering between them.

other info that might help us debug this:
Possible interrupt unsafe locking scenario:

Quoted 8 lines

       CPU0                    CPU1
       ----                    ----
  lock(&mm->mmap_sem);
                               local_irq_disable();
                               lock(&xfs_dir_ilock_class);
                               lock(&mm->mmap_sem);
  <Interrupt>
    lock(&xfs_dir_ilock_class);

Well, that's rather interesting. I'd love to know where we take the directory ilock with interupts disabled - and then take a page fault... :/

Quoted 42 lines

Realistically, I think that the readdir locking change was fundamentally broken. Why? Because taking page faults while holding the ilock violates the lock order we have for inodes. For regular files, the lock heirarchy is iolock -> page fault -> ilock, and we got rid of the need for the iolock in the inode reclaim path because of all the problems it caused lockdep. Now we've gone and reintroduced that same set of problems - only worse - by allowing the readdir code to take page faults while holding the ilock....

I'd like to revert the change that introduced the ilock into the readdir path, but I suspect that woul dbe an unpopular direction to take. However, we need a solution that doesn't drive us insane with lockdep false positives.

IOWs, we need to do this differently - readdir needs to be protected by the iolock, not the ilock, and only the block mapping routines in the readdir code should be protected by the ilock. i.e. the same locking strategy that is used for regular files: iolock protects the contents of the file, the ilock protects the inode metadata.

What I think we really need to do is drive the ilock all the way into the block mapping functions of the directory code and to the places where the inode metadata is actually going to be modified. The iolock protects access to the directory data, and logging of changes to those blocks is serialised by the buffer locks, so we don't actually need the inode ilock to serialise access to the directory data - we only need the ilock for modifications to the inode and it's blockmaps itself, same as for regular files.

Changing the directory code to handle this sort of locking is going to require a bit of surgery. However, I can see advantages to moving directory data to the same locking strategy as regular file data - locking heirarchies are identical, directory ilock hold times are much reduced, we don't get lockdep whining about taking page faults with the ilock held, etc.

A quick hack at to demonstrate the high level, initial step of using the IOLOCK for readdir serialisation. I've done a little smoke testing on it, so it won't die immediately. It should get rid of all the nasty lockdep issues, but it doesn't start to address the deeper restructing that is needed.

Quoted 4 lines

Thoughts, comments? I'm especially interested in what SGI people have to say here because this locking change was needed for CXFS, and changing the directory locking iheirarchy is probably going to upset CXFS a lot...

Protecting directory contents with the iolock and pushing the ilock down to protect the directory block mapping functions seems like a good strategy to me.

In this area I think it's good to treat directories the
same as regular files. I have not had a very close look yet... and couldn't find where we take a fault with irqs disabled. Maybe something out of a workqueue?

Quoted 44 lines · Ben Myers

Hi Dave,

Quoted 41 lines · Dave Chinner

Quoted 28 lines · Michael L. Semon

Hi! I was playing a shell game with some precious backup data. It went like this:

open a 36-GB source partition (DM linear, partitions from 2 drives,
v5-superblock XFS)

open a 32-GB aes-xts crypt (v4-superblock XFS)

cp -av from holding partition to crypt

It was during the cp operation that I got this multi-CPU version of the harmless lockdep:

=========================================================
[ INFO: possible irq lock inversion dependency detected ] 3.14.0-rc5+ #6 Not tainted
---------------------------------------------------------
kswapd0/25 just changed the state of lock:
(&xfs_dir_ilock_class){++++-.}, at: [<791c09fa>] xfs_ilock+0xff/0x16c
but this lock took another, RECLAIM_FS-unsafe lock in the past:
(&mm->mmap_sem){++++++}

and interrupts could create inverse lock ordering between them.

other info that might help us debug this:
Possible interrupt unsafe locking scenario:

Quoted 8 lines

       CPU0                    CPU1
       ----                    ----
  lock(&mm->mmap_sem);
                               local_irq_disable();
                               lock(&xfs_dir_ilock_class);
                               lock(&mm->mmap_sem);
  <Interrupt>
    lock(&xfs_dir_ilock_class);

Well, that's rather interesting. I'd love to know where we take the directory ilock with interupts disabled - and then take a page fault... :/

.....

Quoted 47 lines

Quoted 46 lines

Realistically, I think that the readdir locking change was fundamentally broken. Why? Because taking page faults while holding the ilock violates the lock order we have for inodes. For regular files, the lock heirarchy is iolock -> page fault -> ilock, and we got rid of the need for the iolock in the inode reclaim path because of all the problems it caused lockdep. Now we've gone and reintroduced that same set of problems - only worse - by allowing the readdir code to take page faults while holding the ilock....

I'd like to revert the change that introduced the ilock into the readdir path, but I suspect that woul dbe an unpopular direction to take. However, we need a solution that doesn't drive us insane with lockdep false positives.

IOWs, we need to do this differently - readdir needs to be protected by the iolock, not the ilock, and only the block mapping routines in the readdir code should be protected by the ilock. i.e. the same locking strategy that is used for regular files: iolock protects the contents of the file, the ilock protects the inode metadata.

What I think we really need to do is drive the ilock all the way into the block mapping functions of the directory code and to the places where the inode metadata is actually going to be modified. The iolock protects access to the directory data, and logging of changes to those blocks is serialised by the buffer locks, so we don't actually need the inode ilock to serialise access to the directory data - we only need the ilock for modifications to the inode and it's blockmaps itself, same as for regular files.

Changing the directory code to handle this sort of locking is going to require a bit of surgery. However, I can see advantages to moving directory data to the same locking strategy as regular file data - locking heirarchies are identical, directory ilock hold times are much reduced, we don't get lockdep whining about taking page faults with the ilock held, etc.

A quick hack at to demonstrate the high level, initial step of using the IOLOCK for readdir serialisation. I've done a little smoke testing on it, so it won't die immediately. It should get rid of all the nasty lockdep issues, but it doesn't start to address the deeper restructing that is needed.

Thoughts, comments? I'm especially interested in what SGI people have to say here because this locking change was needed for CXFS, and changing the directory locking iheirarchy is probably going to upset CXFS a lot...

Quoted 3 lines

Protecting directory contents with the iolock and pushing the ilock down to protect the directory block mapping functions seems like a good strategy to me.

Ok, it seems like a good strategy, but does it work for CXFS?

What about the other option of a combination of ilock for mapping and read, and buffer locks for stabilisation?

Quoted 4 lines

In this area I think it's good to treat directories the
same as regular files. I have not had a very close look yet... and couldn't find where we take a fault with irqs disabled. Maybe something out of a workqueue?

Workqueues don't run with interrupts disabled. Yes, we take inode locks in workqueue context (e.g. AIL tail pushing), but that's not what is being warned about here. And the page fault only comes from the filldir callback which runs copy_to_user(), and that most definitely does not occur in interrupt context.

FWIW, it's invalid to take the ilock in interrupt context because it's a sleeping lock without using trylock semantics. If you use "trylock" semantics, then lockdep doesn't complain about that because it can't deadlock. I *think* that lockdep is complaining about reclaim contexts, not interrupts, and it's issuing this "interrupts disabled" deadlocks because the memory reclaim state code is piggy-backed on the interrupt state tracking within lockdep. Either way, though, lockdep is wrong.