vfs-scale git tree update

Git tree is here:

git://git.kernel.org/pub/scm/linux/kernel/git/npiggin/linux-npiggin.git

Branch is: vfs-scale-working

Head is: b3e19d924b6eaf2ca7d22cba99a517c5171007b6

Changes since last posting:
* Implemented Linus' suggestions:
- Switch the filesystem methods in one hit rather than having _rcu variants. This avoids a lot of cruft from core code.
- Implement filesystem rcu-walk aware conversions in seperate patches after the APIs are changed, making bisectability better.
- Change dentry_memcmp to dentry_cmp
- Rework the way ->d_revalidate is handled to be simpler.

* Fixed configfs bug caused by switching d_op on live dentry.

* Leave d_revalidate filesystem changes out for now. The sysfs one in particular seems to have been causing a funny regression.

* Bit more tweaking of changelogs documentation and comments.

* Passes ltp, xfstests, dbench stressing over nfs, and some of my own racer tests on a couple of filesystems. Needs wider testing.

FYI, when using this code, as merged by Linus, I hit the BUG_ON() at the beginning of d_set_d_op() when it's called by autofs4_dir_mkdir().  I managed to work around it by just commenting out those BUG_ON() calls but it's something that ought to get addressed properly.

Quoted 6 lines · Alex Elder

FYI, when using this code, as merged by Linus, I hit the BUG_ON() at the beginning of d_set_d_op() when it's called by autofs4_dir_mkdir(). I managed to work around it by just commenting out those BUG_ON() calls but it's something that ought to get addressed properly.

Yeah, removing the BUG_ON() isn't the right thing to do - it means that autofs4 is obviously setting the dentry ops twice for the same dentry.

Possibly the thing could be relaxed to allow setting the _same_ d_op pointer, ie do something like

   if (dentry->d_op == op)
      return;

at the top of that function. But looking at it, I don't think that fixes the autofs4 issue.

The fact that autofs4 does "d_add()" before it sets the d_ops (or other dentry state, for that matter) looks a bit scary. To me that smells like it might get a dentry lookup hit before it's actually fully done.

Does it make any difference if you move the various d_add() calls down to the end of the functions to when the "dentry" has really been instantiated?

Looking at it quickly, I don't think that would matter for the case at hand.  I.e., that might be safer but it doesn't address the fact that these fields are getting initialized multiple times.

[ Added Ian to cc - the BUG_ON() in d_set_d_op() triggers on autofs4, see lkml thread for more details if you care ]

Quoted 5 lines · Alex Elder

Looking at it quickly, I don't think that would matter for the case at hand.  I.e., that might be safer but it doesn't address the fact that these fields are getting initialized multiple times.

You're right - without looking closer or thinking about it any more, I was thinking mkdir etc would create the new dentry, but it can easily be a pre-existing negative one. So it's not like mkdir is always going to be something new that can just be initialized.

So yeah, no race or anything like that required - just regular acceses.

That said, if we make sure that the d_op gets cleared (or the dentry dropped) when something turns negative, and a negative lookup never sets d_op, it should all basically work.

Can you figure out the exact sequence that causes this? Is it enough to just mount something once?

Quoted 12 lines · Linus Torvalds

Quoted 6 lines · Alex Elder

FYI, when using this code, as merged by Linus, I hit the BUG_ON() at the beginning of d_set_d_op() when it's called by autofs4_dir_mkdir(). I managed to work around it by just commenting out those BUG_ON() calls but it's something that ought to get addressed properly.

Yeah, removing the BUG_ON() isn't the right thing to do - it means that autofs4 is obviously setting the dentry ops twice for the same dentry.

Quoted 8 lines

Possibly the thing could be relaxed to allow setting the _same_ d_op pointer, ie do something like

   if (dentry->d_op == op)
      return;

at the top of that function. But looking at it, I don't think that fixes the autofs4 issue.

That's easy enough, but it seems everybody else ensures this gets done just once per dentry, and it would be nice to preserve that "tightness" if possible.

Quoted 4 lines

The fact that autofs4 does "d_add()" before it sets the d_ops (or other dentry state, for that matter) looks a bit scary. To me that smells like it might get a dentry lookup hit before it's actually fully done.

Agreed.

Quoted 4 lines

Does it make any difference if you move the various d_add() calls down to the end of the functions to when the "dentry" has really been instantiated?

Looking at it quickly, I don't think that would matter for the case at hand. I.e., that might be safer but it doesn't address the fact that these fields are getting initialized multiple times.

-Alex

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Quoted 26 lines · Alex Elder

Quoted 12 lines · Linus Torvalds

Quoted 6 lines · Alex Elder

FYI, when using this code, as merged by Linus, I hit the BUG_ON() at the beginning of d_set_d_op() when it's called by autofs4_dir_mkdir().  I managed to work around it by just commenting out those BUG_ON() calls but it's something that ought to get addressed properly.

Yeah, removing the BUG_ON() isn't the right thing to do - it means that autofs4 is obviously setting the dentry ops twice for the same dentry.

Quoted 8 lines

Possibly the thing could be relaxed to allow setting the _same_ d_op pointer, ie do something like

   if (dentry->d_op == op)
      return;

at the top of that function. But looking at it, I don't think that fixes the autofs4 issue.

That's easy enough, but it seems everybody else ensures this gets done just once per dentry, and it would be nice to preserve that "tightness" if possible.

Quoted 6 lines

Quoted 4 lines

The fact that autofs4 does "d_add()" before it sets the d_ops (or other dentry state, for that matter) looks a bit scary. To me that smells like it might get a  dentry lookup hit before it's actually fully done.

Agreed.

Isn't the parent i_mutex held during mkdir()?

Still the order can be changed, of course.

Quoted 5 lines

Quoted 4 lines

Does it make any difference if you move the various d_add() calls down to the end of the functions to when the "dentry" has really been instantiated?

Quoted 4 lines

Looking at it quickly, I don't think that would matter for the case at hand.  I.e., that might be safer but it doesn't address the fact that these fields are getting initialized multiple times.

Yeah, a hangover from changes done over time. Not setting the dentry op in ->lookup() should fix this.

Isn't the parent i_mutex held during mkdir()?

Quoted 4 lines

Looking at it quickly, I don't think that would matter for the case at hand.  I.e., that might be safer but it doesn't address the fact that these fields are getting initialized multiple times.

Yeah, a hangover from changes done over time. Not setting the dentry op in ->lookup() should fix this.

Quoted 2 lines · Ian Kent

Isn't the parent i_mutex held during mkdir()?

Yes, but a lookup that hits in the dentry cache won't actually take the parent mutex.

So as far as I can tell, doing the "d_add()" before setting d_op can result in another CPU coming in and seeing the newly added dentry before d_op has actually been initialized. Exactly because it will do the dentry lookup without holding any mutex.

Of course, it's a very small window, so it probably doesn't matter in practice.

Quoted 5 lines

Quoted 4 lines

Looking at it quickly, I don't think that would matter for the case at hand. I.e., that might be safer but it doesn't address the fact that these fields are getting initialized multiple times.

Quoted 2 lines

Yeah, a hangover from changes done over time. Not setting the dentry op in ->lookup() should fix this.

Alex, care to test just removing the d_set_d_op() call from autofs4_lookup()?

(That code is a bit scary, though - it explicitly makes it a negative dentry with a d_instantiate(dentry, NULL), and then hides the inode information away separately. Scary scary)

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 24 lines · Linus Torvalds

Quoted 2 lines · Ian Kent

Isn't the parent i_mutex held during mkdir()?

Yes, but a lookup that hits in the dentry cache won't actually take the parent mutex.

So as far as I can tell, doing the "d_add()" before setting d_op can result in another CPU coming in and seeing the newly added dentry before d_op has actually been initialized. Exactly because it will do the dentry lookup without holding any mutex.

Of course, it's a very small window, so it probably doesn't matter in practice.

Quoted 7 lines

Quoted 4 lines

Looking at it quickly, I don't think that would matter for the case at hand. I.e., that might be safer but it doesn't address the fact that these fields are getting initialized multiple times.

Yeah, a hangover from changes done over time. Not setting the dentry op in ->lookup() should fix this.

Alex, care to test just removing the d_set_d_op() call from autofs4_lookup()?

Quoted 3 lines

(That code is a bit scary, though - it explicitly makes it a negative dentry with a d_instantiate(dentry, NULL), and then hides the inode information away separately. Scary scary)

Yeah, but the expire to mount races with autofs are difficult to handle and this approach has worked well under heavy stress testing. It's true that this would almost certainly be bad for a file system that supported a full range of functionality but that's not so for autofs.

Quoted 8 lines · Ian Kent

Quoted 7 lines · Linus Torvalds

Quoted 2 lines · Ian Kent

Yeah, a hangover from changes done over time. Not setting the dentry op in ->lookup() should fix this.

Alex, care to test just removing the d_set_d_op() call from autofs4_lookup()?

Quoted 8 lines

Quoted 3 lines

(That code is a bit scary, though - it explicitly makes it a negative dentry with a d_instantiate(dentry, NULL), and then hides the inode information away separately. Scary scary)

Yeah, but the expire to mount races with autofs are difficult to handle and this approach has worked well under heavy stress testing. It's true that this would almost certainly be bad for a file system that supported a full range of functionality but that's not so for autofs.

I think I have to partly take this back.
With Nick's recent vfs-scale patches this may not be OK any more since the dcache_lock has gone away and, at first glance, it looks like the added autofs4_lock spin lock doesn't provide the needed protection.

Quoted 18 lines · Ian Kent

Quoted 16 lines · Ian Kent

Quoted 10 lines · Linus Torvalds

Quoted 2 lines · Ian Kent

Yeah, a hangover from changes done over time. Not setting the dentry op in ->lookup() should fix this.

Alex, care to test just removing the d_set_d_op() call from autofs4_lookup()?

(That code is a bit scary, though - it explicitly makes it a negative dentry with a d_instantiate(dentry, NULL), and then hides the inode information away separately. Scary scary)

Yeah, but the expire to mount races with autofs are difficult to handle and this approach has worked well under heavy stress testing. It's true that this would almost certainly be bad for a file system that supported a full range of functionality but that's not so for autofs.

Quoted 4 lines

I think I have to partly take this back.
With Nick's recent vfs-scale patches this may not be OK any more since the dcache_lock has gone away and, at first glance, it looks like the added autofs4_lock spin lock doesn't provide the needed protection.

Hm, what are the concurrencies that you need protection from?

Quoted 1 line · Nick Piggin

Hm, what are the concurrencies that you need protection from?

Ha, I think I'm wrong about this, after looking more closely at this I'm struggling to see why autofs4_lock is needed at all.

Quoted 5 lines · Ian Kent

Quoted 1 line · Nick Piggin

Hm, what are the concurrencies that you need protection from?

Ha, I think I'm wrong about this, after looking more closely at this I'm struggling to see why autofs4_lock is needed at all.

Well you did send me a series of patches to remove it, but unfortunately that was just as you made some larger changes to autofs4 upstream and I wasn't able to keep them up to date.

It would be much appreciated if you had time to take another look at all the locking and streamline it.

Quoted 7 lines · Nick Piggin

Quoted 5 lines · Ian Kent

Quoted 1 line · Nick Piggin

Hm, what are the concurrencies that you need protection from?

Ha, I think I'm wrong about this, after looking more closely at this I'm struggling to see why autofs4_lock is needed at all.

Quoted 6 lines

Well you did send me a series of patches to remove it, but unfortunately that was just as you made some larger changes to autofs4 upstream and I wasn't able to keep them up to date.

It would be much appreciated if you had time to take another look at all the locking and streamline it.

I have started looking at it but just the autofs4_lock.

As you know, we have some significant autofs changes in progress, so I don't want to spend a huge amount of time testing (and the testing does take a long time) against source that will be very different. So far I can't see that the autofs4_lock will introduce any problem so I want to leave it for now and (probably) eliminate it in the new code since that will need changes as well and will need to be re-tested.

I'd appreciate it if you could find time to reply to David's questions about the changes to our d_automount patch series. Although, based on our previous discussion, I think he has it about right, but a word or two from you would be really helpful.

Quoted 26 lines · Alex Elder

Quoted 12 lines · Linus Torvalds

Quoted 6 lines · Alex Elder

FYI, when using this code, as merged by Linus, I hit the BUG_ON() at the beginning of d_set_d_op() when it's called by autofs4_dir_mkdir(). I managed to work around it by just commenting out those BUG_ON() calls but it's something that ought to get addressed properly.

Yeah, removing the BUG_ON() isn't the right thing to do - it means that autofs4 is obviously setting the dentry ops twice for the same dentry.

Quoted 8 lines

Possibly the thing could be relaxed to allow setting the _same_ d_op pointer, ie do something like

   if (dentry->d_op == op)
      return;

at the top of that function. But looking at it, I don't think that fixes the autofs4 issue.

That's easy enough, but it seems everybody else ensures this gets done just once per dentry, and it would be nice to preserve that "tightness" if possible.

Quoted 6 lines

Quoted 4 lines

The fact that autofs4 does "d_add()" before it sets the d_ops (or other dentry state, for that matter) looks a bit scary. To me that smells like it might get a dentry lookup hit before it's actually fully done.

Agreed.

Isn't the parent i_mutex held during mkdir()?
Still the order can be changed, of course.

Quoted 5 lines

Quoted 4 lines

Does it make any difference if you move the various d_add() calls down to the end of the functions to when the "dentry" has really been instantiated?

Quoted 4 lines

Looking at it quickly, I don't think that would matter for the case at hand. I.e., that might be safer but it doesn't address the fact that these fields are getting initialized multiple times.

Yeah, a hangover from changes done over time. Not setting the dentry op in ->lookup() should fix this.

Quoted 47 lines · Ian Kent

Quoted 26 lines · Alex Elder

Quoted 12 lines · Linus Torvalds

Quoted 6 lines · Alex Elder

FYI, when using this code, as merged by Linus, I hit the BUG_ON() at the beginning of d_set_d_op() when it's called by autofs4_dir_mkdir(). I managed to work around it by just commenting out those BUG_ON() calls but it's something that ought to get addressed properly.

Yeah, removing the BUG_ON() isn't the right thing to do - it means that autofs4 is obviously setting the dentry ops twice for the same dentry.

Quoted 8 lines

Possibly the thing could be relaxed to allow setting the _same_ d_op pointer, ie do something like

   if (dentry->d_op == op)
      return;

at the top of that function. But looking at it, I don't think that fixes the autofs4 issue.

That's easy enough, but it seems everybody else ensures this gets done just once per dentry, and it would be nice to preserve that "tightness" if possible.

Quoted 6 lines

Quoted 4 lines

The fact that autofs4 does "d_add()" before it sets the d_ops (or other dentry state, for that matter) looks a bit scary. To me that smells like it might get a dentry lookup hit before it's actually fully done.

Agreed.

Isn't the parent i_mutex held during mkdir()?
Still the order can be changed, of course.

Quoted 9 lines

Quoted 4 lines

Does it make any difference if you move the various d_add() calls down to the end of the functions to when the "dentry" has really been instantiated?

Looking at it quickly, I don't think that would matter for the case at hand. I.e., that might be safer but it doesn't address the fact that these fields are getting initialized multiple times.

Quoted 2 lines

Yeah, a hangover from changes done over time. Not setting the dentry op in ->lookup() should fix this.

Could you try this patch please.

autofs4 - dont set dentry op in ->lookup()

Quoted 48 lines · Ian Kent

Quoted 47 lines · Ian Kent

Quoted 26 lines · Alex Elder

Quoted 12 lines · Linus Torvalds

Quoted 6 lines · Alex Elder

FYI, when using this code, as merged by Linus, I hit the BUG_ON() at the beginning of d_set_d_op() when it's called by autofs4_dir_mkdir(). I managed to work around it by just commenting out those BUG_ON() calls but it's something that ought to get addressed properly.

Yeah, removing the BUG_ON() isn't the right thing to do - it means that autofs4 is obviously setting the dentry ops twice for the same dentry.

Quoted 8 lines

Possibly the thing could be relaxed to allow setting the _same_ d_op pointer, ie do something like

   if (dentry->d_op == op)
      return;

at the top of that function. But looking at it, I don't think that fixes the autofs4 issue.

That's easy enough, but it seems everybody else ensures this gets done just once per dentry, and it would be nice to preserve that "tightness" if possible.

Quoted 6 lines

Quoted 4 lines

The fact that autofs4 does "d_add()" before it sets the d_ops (or other dentry state, for that matter) looks a bit scary. To me that smells like it might get a dentry lookup hit before it's actually fully done.

Agreed.

Isn't the parent i_mutex held during mkdir()?
Still the order can be changed, of course.

Quoted 9 lines

Quoted 4 lines

Does it make any difference if you move the various d_add() calls down to the end of the functions to when the "dentry" has really been instantiated?

Looking at it quickly, I don't think that would matter for the case at hand. I.e., that might be safer but it doesn't address the fact that these fields are getting initialized multiple times.

Quoted 4 lines

Quoted 2 lines

Yeah, a hangover from changes done over time. Not setting the dentry op in ->lookup() should fix this.

Could you try this patch please.

OK, sorry for the delay. I tried the patch. I applied it against 4162cf64973df51fc885825bc9ca4d055891c49f, which is the linus/master branch I had on hand. This time I got a different failure due to a null pointer dereference. Console capture below. I can log in still but the boot sequence never got to the login prompt on the console as it normally does.

-Alex

Quoted 1 line

autofs4 - dont set dentry op in ->lookup()

. . .
Starting smartd
done
Starting network time protocol daemon (NTPD)
done
Starting automount BUG: unable to handle kernel NULL pointer dereference at 0000000000000010
IP: [<ffffffffa0296744>] autofs4_lookup+0x3dd/0x4a7 [autofs4] PGD 0

Oops: 0000 [#1] SMP 
last sysfs
file: /sys/devices/pci0000:00/0000:00:1e.0/0000:08:01.0/local_cpus
CPU 2 
Modules linked in: autofs4 binfmt_misc nfs lockd nfs_acl auth_rpcgss

sunrpc ib_ipoib ib_cm ib_sa ipv6 mperf ib_uverbs ib_umad iw_cxgb3 cxgb3 mdio mlx4_ib mlx4_core microcode fuse loop dm_mod sr_mod cdrom ib_mthca ib_mad e1000e mptfc scsi_transport_fc scsi_tgt ib_core shpchp usb_storage i5k_amb pci_hotplug rtc_cmos rtc_core iTCO_wdt rtc_lib tpm_tis i2c_i801 i2c_core ioatdma dca tpm i5000_edac iTCO_vendor_support edac_core pcspkr container button joydev tpm_bios serio_raw sg usbhid hid uhci_hcd ehci_hcd sd_mod crc_t10dif usbcore ext3 jbd mbcache piix edd xfs exportfs fan thermal processor thermal_sys hwmon ide_generic ide_core sata_nv megaraid_sas mptsas mptscsih mptbase scsi_transport_sas ata_piix ahci libahci libata scsi_mod

Pid: 3880, comm: automount Not tainted 2.6.37-alex #1 X7DGT-INF/AltixXE310

RIP: 0010:[<ffffffffa0296744>]  [<ffffffffa0296744>] autofs4_lookup
+0x3dd/0x4a7 [autofs4]
RSP: 0018:ffff88011f7a3b88  EFLAGS: 00010202
RAX: 0000000000000002 RBX: ffff880117faeb40 RCX: 0000000000000011
RDX: 0000000000000000 RSI: ffff88011ecfcb38 RDI: ffff88011ecfcb38
RBP: ffff88011f7a3c38 R08: 0000000000000000 R09: ffff88011ed41578
R10: ffff880120f4c007 R11: 000000000000343e R12: ffff880117fae9f8
R13: ffff88011ecfcb00 R14: 0000000000000001 R15: ffff88011ecfcb00
FS:  00007ffb9915a950(0000) GS:ffff8800cfd00000(0000)

knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000010 CR3: 000000011e7d1000 CR4: 00000000000006e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400 Process automount (pid: 3880, threadinfo ffff88011f7a2000, task ffff880124628480)

Stack:
 ffff88011f7a3c08 0000000000000011 ffff88012ffd9e00 ffff88011f7a3db8
 ffff880120247050 ffff8800cfd0e0e0 ffff880117faeb40 0000000000000000
 00000001249405f8 ffff880117faebc0 ffff88011ecfcb38 00000011dc2bc535
Call Trace:
 [<ffffffff810e86fb>] ? d_alloc+0x165/0x1bd
 [<ffffffff810de19e>] d_alloc_and_lookup+0x49/0x68
 [<ffffffff810de34c>] do_lookup+0x18f/0x23a
 [<ffffffff810e07a9>] link_path_walk+0x5ad/0xa90
 [<ffffffff810e0edd>] do_path_lookup+0x4b/0x107
 [<ffffffff810e1973>] user_path_at+0x52/0x8c
 [<ffffffff810d91fe>] ? cp_new_stat+0xe2/0xee
 [<ffffffff810e7525>] ? dput+0x25/0x23e
 [<ffffffff810d93bc>] vfs_fstatat+0x35/0x62
 [<ffffffff810d94cb>] vfs_stat+0x16/0x18
 [<ffffffff810d94e7>] sys_newstat+0x1a/0x3b
 [<ffffffff8100293b>] system_call_fastpath+0x16/0x1b
Code: 47 60 48 85 c0 74 14 48 8b 00 48 85 c0 74 0c 48 8b b5 68 ff ff ff

4c 89 ff ff d0 48 8b bd 78 ff ff ff e8 79 c1 03 e1 48 8b 55 88 <f6> 42 10 04 74 76 65 48 8b 14 25 80 b5 00 00 48 8b 42 08 48 8b RIP [<ffffffffa0296744>] autofs4_lookup+0x3dd/0x4a7 [autofs4]
RSP <ffff88011f7a3b88>
CR2: 0000000000000010

---[ end trace 6655d1f57e42cf9e ]---

Starting mail service (Postfix)
done
Starting CRON daemon
done
eth0: no IPv6 routers present

Quoted 54 lines · Alex Elder

Quoted 48 lines · Ian Kent

Quoted 47 lines · Ian Kent

Quoted 26 lines · Alex Elder

Quoted 12 lines · Linus Torvalds

Quoted 6 lines · Alex Elder

FYI, when using this code, as merged by Linus, I hit the BUG_ON() at the beginning of d_set_d_op() when it's called by autofs4_dir_mkdir(). I managed to work around it by just commenting out those BUG_ON() calls but it's something that ought to get addressed properly.

Yeah, removing the BUG_ON() isn't the right thing to do - it means that autofs4 is obviously setting the dentry ops twice for the same dentry.

Quoted 8 lines

Possibly the thing could be relaxed to allow setting the _same_ d_op pointer, ie do something like

   if (dentry->d_op == op)
      return;

at the top of that function. But looking at it, I don't think that fixes the autofs4 issue.

That's easy enough, but it seems everybody else ensures this gets done just once per dentry, and it would be nice to preserve that "tightness" if possible.

Quoted 6 lines

Quoted 4 lines

The fact that autofs4 does "d_add()" before it sets the d_ops (or other dentry state, for that matter) looks a bit scary. To me that smells like it might get a dentry lookup hit before it's actually fully done.

Agreed.

Isn't the parent i_mutex held during mkdir()?
Still the order can be changed, of course.

Quoted 9 lines

Quoted 4 lines

Does it make any difference if you move the various d_add() calls down to the end of the functions to when the "dentry" has really been instantiated?

Looking at it quickly, I don't think that would matter for the case at hand. I.e., that might be safer but it doesn't address the fact that these fields are getting initialized multiple times.

Quoted 4 lines

Quoted 2 lines

Yeah, a hangover from changes done over time. Not setting the dentry op in ->lookup() should fix this.

Could you try this patch please.

Quoted 7 lines

OK, sorry for the delay. I tried the patch. I applied it against 4162cf64973df51fc885825bc9ca4d055891c49f, which is the linus/master branch I had on hand. This time I got a different failure due to a null pointer dereference. Console capture below. I can log in still but the boot sequence never got to the login prompt on the console as it normally does.

Sorry, that is rather an obvious mistake on my part. There's a call to d_op->d_revalidate() just below where the dentry operations are set. I'm tempted to just call the revalidate function directly since it is always called anyway. But let me check we've done in the our current of tree autofs patch series first.

Quoted 3 lines · Ian Kent

Quoted 2 lines · Alex Elder

Quoted 1 line · Ian Kent

On Wed, 2011-01-12 at 11:59 +0800, Ian Kent wrote:

Quoted 11 lines

Quoted 9 lines

Quoted 1 line

Could you try this patch please.

OK, sorry for the delay. I tried the patch. I applied it against 4162cf64973df51fc885825bc9ca4d055891c49f, which is the linus/master branch I had on hand. This time I got a different failure due to a null pointer dereference. Console capture below. I can log in still but the boot sequence never got to the login prompt on the console as it normally does.

Quoted 5 lines

Sorry, that is rather an obvious mistake on my part. There's a call to d_op->d_revalidate() just below where the dentry operations are set. I'm tempted to just call the revalidate function directly since it is always called anyway. But let me check we've done in the our current of tree autofs patch series first.

Ha, there is no revalidate in the current not yet merged autofs. So I think we just need to work around it.

Try this and see if it resolves the issue.

autofs4 - set dentry op in ->lookup() only

From: Ian Kent <[email protected]>

With the introduction of the vfs-scale patch series setting dentry operations more than once (or changing them) triggers a BUG_ON(). Since the two dentry operations used are the same, just set the operations in ->lookup() and remove the set in ->symlink() and ->mkdir().

Also, move the d_add() in ->symlink() and ->mkdir() to the end of the function.
---

 fs/autofs4/root.c |   40 +++++++---------------------------------
 1 files changed, 7 insertions(+), 33 deletions(-)

fs/autofs4/root.c +7 -33

index 651e4ef..0897706 100644
@@ -419,12 +419,6 @@ void autofs4_dentry_release(struct dentry *de)
 	}
 }
 
-/* For dentries of directories in the root dir */
-static const struct dentry_operations autofs4_root_dentry_operations = {
-	.d_revalidate	= autofs4_revalidate,
-	.d_release	= autofs4_dentry_release,
-};
-
 /* For other dentries */
 static const struct dentry_operations autofs4_dentry_operations = {
 	.d_revalidate	= autofs4_revalidate,

@@ -568,19 +562,6 @@ static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, s
 		ino = autofs4_dentry_ino(dentry);
 	} else {
 		/*
-		 * Mark the dentry incomplete but don't hash it. We do this
-		 * to serialize our inode creation operations (symlink and
-		 * mkdir) which prevents deadlock during the callback to
-		 * the daemon. Subsequent user space lookups for the same
-		 * dentry are placed on the wait queue while the daemon
-		 * itself is allowed passage unresticted so the create
-		 * operation itself can then hash the dentry. Finally,
-		 * we check for the hashed dentry and return the newly
-		 * hashed dentry.
-		 */
-		d_set_d_op(dentry, &autofs4_root_dentry_operations);
-
-		/*
 		 * And we need to ensure that the same dentry is used for
 		 * all following lookup calls until it is hashed so that
 		 * the dentry flags are persistent throughout the request.

@@ -589,6 +570,8 @@ static struct dentry *autofs4_lookup(struct inode *dir, struct dentry *dentry, s
 		if (!ino)
 			return ERR_PTR(-ENOMEM);
 
+		d_set_d_op(dentry, &autofs4_dentry_operations);
+
 		dentry->d_fsdata = ino;
 		ino->dentry = dentry;
 

@@ -714,12 +697,6 @@ static int autofs4_dir_symlink(struct inode *dir,
 			kfree(ino);
 		return -ENOMEM;
 	}
-	d_add(dentry, inode);
-
-	if (dir == dir->i_sb->s_root->d_inode)
-		d_set_d_op(dentry, &autofs4_root_dentry_operations);
-	else
-		d_set_d_op(dentry, &autofs4_dentry_operations);
 
 	dentry->d_fsdata = ino;
 	ino->dentry = dget(dentry);

@@ -732,6 +709,8 @@ static int autofs4_dir_symlink(struct inode *dir,
 	ino->u.symlink = cp;
 	dir->i_mtime = CURRENT_TIME;
 
+	d_add(dentry, inode);
+
 	return 0;
 }
 

@@ -849,12 +828,6 @@ static int autofs4_dir_mkdir(struct inode *dir, struct dentry *dentry, int mode)
 			kfree(ino);
 		return -ENOMEM;
 	}
-	d_add(dentry, inode);
-
-	if (dir == dir->i_sb->s_root->d_inode)
-		d_set_d_op(dentry, &autofs4_root_dentry_operations);
-	else
-		d_set_d_op(dentry, &autofs4_dentry_operations);
 
 	dentry->d_fsdata = ino;
 	ino->dentry = dget(dentry);

@@ -866,6 +839,8 @@ static int autofs4_dir_mkdir(struct inode *dir, struct dentry *dentry, int mode)
 	inc_nlink(dir);
 	dir->i_mtime = CURRENT_TIME;
 
+	d_add(dentry, inode);
+
 	return 0;
 }
 

@@ -944,8 +919,7 @@ static inline int autofs4_ask_umount(struct vfsmount *mnt, int __user *p)
 int is_autofs4_dentry(struct dentry *dentry)
 {
 	return dentry && dentry->d_inode &&
-		(dentry->d_op == &autofs4_root_dentry_operations ||
-		 dentry->d_op == &autofs4_dentry_operations) &&
+		(dentry->d_op == &autofs4_dentry_operations) &&
 		dentry->d_fsdata != NULL;
 }
 

Quoted 26 lines · Alex Elder

Quoted 12 lines · Linus Torvalds

Quoted 6 lines · Alex Elder

FYI, when using this code, as merged by Linus, I hit the BUG_ON() at the beginning of d_set_d_op() when it's called by autofs4_dir_mkdir(). I managed to work around it by just commenting out those BUG_ON() calls but it's something that ought to get addressed properly.

Yeah, removing the BUG_ON() isn't the right thing to do - it means that autofs4 is obviously setting the dentry ops twice for the same dentry.

Quoted 8 lines

Possibly the thing could be relaxed to allow setting the _same_ d_op pointer, ie do something like

   if (dentry->d_op == op)
      return;

at the top of that function. But looking at it, I don't think that fixes the autofs4 issue.

That's easy enough, but it seems everybody else ensures this gets done just once per dentry, and it would be nice to preserve that "tightness" if possible.

Quoted 6 lines

Quoted 4 lines

The fact that autofs4 does "d_add()" before it sets the d_ops (or other dentry state, for that matter) looks a bit scary. To me that smells like it might get a dentry lookup hit before it's actually fully done.

Agreed.

Isn't the parent i_mutex held during mkdir()?
Still the order can be changed, of course.

Quoted 5 lines

Quoted 4 lines

Does it make any difference if you move the various d_add() calls down to the end of the functions to when the "dentry" has really been instantiated?

Quoted 4 lines

Looking at it quickly, I don't think that would matter for the case at hand. I.e., that might be safer but it doesn't address the fact that these fields are getting initialized multiple times.

Yeah, a hangover from changes done over time. Not setting the dentry op in ->lookup() should fix this.

Quoted 47 lines · Ian Kent

Quoted 26 lines · Alex Elder

Quoted 12 lines · Linus Torvalds

Quoted 6 lines · Alex Elder

FYI, when using this code, as merged by Linus, I hit the BUG_ON() at the beginning of d_set_d_op() when it's called by autofs4_dir_mkdir(). I managed to work around it by just commenting out those BUG_ON() calls but it's something that ought to get addressed properly.

Yeah, removing the BUG_ON() isn't the right thing to do - it means that autofs4 is obviously setting the dentry ops twice for the same dentry.

Quoted 8 lines

Possibly the thing could be relaxed to allow setting the _same_ d_op pointer, ie do something like

   if (dentry->d_op == op)
      return;

at the top of that function. But looking at it, I don't think that fixes the autofs4 issue.

That's easy enough, but it seems everybody else ensures this gets done just once per dentry, and it would be nice to preserve that "tightness" if possible.

Quoted 6 lines

Quoted 4 lines

The fact that autofs4 does "d_add()" before it sets the d_ops (or other dentry state, for that matter) looks a bit scary. To me that smells like it might get a dentry lookup hit before it's actually fully done.

Agreed.

Isn't the parent i_mutex held during mkdir()?
Still the order can be changed, of course.

Quoted 9 lines

Quoted 4 lines

Does it make any difference if you move the various d_add() calls down to the end of the functions to when the "dentry" has really been instantiated?

Looking at it quickly, I don't think that would matter for the case at hand. I.e., that might be safer but it doesn't address the fact that these fields are getting initialized multiple times.

Quoted 3 lines

Yeah, a hangover from changes done over time. Not setting the dentry op in ->lookup() should fix this.

for 9p I did the other way round. lookup now set the dentry op for both negative and positive dentries and rest of the place we dropped d_set_d_op (b8b80cf37c7f0e32729262f805bc0fa81c3e9d12 have the details)

Quoted 3 lines · Aneesh Kumar K. V

for 9p I did the other way round. lookup now set the dentry op for both negative and positive dentries and rest of the place we dropped d_set_d_op (b8b80cf37c7f0e32729262f805bc0fa81c3e9d12 have the details)

Sadly I can't do that.

For now I need to avoid calls to ->d_revalidate() as much as possible to reduce a potential deadlock and have access to the flags of the negative dentry (during create) for the purpose of blocking concurrent walks.

Quoted 5 lines · Ian Kent

Quoted 3 lines · Aneesh Kumar K. V

for 9p I did the other way round. lookup now set the dentry op for both negative and positive dentries and rest of the place we dropped d_set_d_op (b8b80cf37c7f0e32729262f805bc0fa81c3e9d12 have the details)

Quoted 5 lines

Sadly I can't do that.

For now I need to avoid calls to ->d_revalidate() as much as possible to reduce a potential deadlock and have access to the flags of the negative dentry (during create) for the purpose of blocking concurrent walks.

What prevents your d_revalidate from just checking for negative dentry?