fs: fix superblock iteration race

Not sure if this is really the _cleanest_ way to fix it. But open coding the list walking is a bit annoying too. And I couldn't see any real way to make the list macro safe. Better ideas?

Quoted 3 lines · Nick Piggin

Not sure if this is really the _cleanest_ way to fix it. But open coding the list walking is a bit annoying too. And I couldn't see any real way to make the list macro safe. Better ideas?

I really think we should open-code the list walking instead. You basically already are doing that, and in a very non-obvious way too (ie you are mixing the non-open-coded list walker with also explicitly playing with the internal variable for that magic walker.

So I would get rid of the 'list_for_each_entry_safe' entirely, and replace it with something like

   struct list_head *list;

   spin_lock(&sb_lock);
   list = super_blocks->next;
   while (list != &super_blocks) {
      struct super_block *sb = list_entry(next, struct super_block, s_list);
      list = list->next;

      if (list_empty(&sb->s_instances))
         continue;

      if (!sb->s_nr_dentry_unused)
         continue;

      sb->s_count++;
      spin_unlock(&sb_lock);

      .... whatever ...

      spin_lock(&sb_lock);
      /* We dropped the lock, need to re-load the next list entry */
      list = sb->s_list.next;
      __put_super(sb);
   }

which isn't that much more complicated, now is it? Sure, it's open-coded, but at least it doesn't play games. And being open-coded, it's a lot more honest about the issue. Maybe even add a comment saying "we can't use the list_for_each[_safe]() macro, because we don't hold the lock and we're not the only ones that may delete things" explaining _why_ it's open-coded.

I dunno. Maybe Al disagrees. I just don't like using the "simple helpers" and then changing subtly how they work by knowing their internals.

Quoted 3 lines · Nick Piggin

Not sure if this is really the _cleanest_ way to fix it. But open coding the list walking is a bit annoying too. And I couldn't see any real way to make the list macro safe. Better ideas?

I really think we should open-code the list walking instead. You basically already are doing that, and in a very non-obvious way too (ie you are mixing the non-open-coded list walker with also explicitly playing with the internal variable for that magic walker.

So I would get rid of the 'list_for_each_entry_safe' entirely, and replace it with something like

   struct list_head *list;

   spin_lock(&sb_lock);
   list = super_blocks->next;
   while (list != &super_blocks) {
      struct super_block *sb = list_entry(next, struct super_block, s_list);
      list = list->next;

      if (list_empty(&sb->s_instances))
         continue;

      if (!sb->s_nr_dentry_unused)
         continue;

      sb->s_count++;
      spin_unlock(&sb_lock);

      .... whatever ...

      spin_lock(&sb_lock);
      /* We dropped the lock, need to re-load the next list entry */
      list = sb->s_list.next;
      __put_super(sb);
   }

which isn't that much more complicated, now is it? Sure, it's open-coded, but at least it doesn't play games. And being open-coded, it's a lot more honest about the issue. Maybe even add a comment saying "we can't use the list_for_each[_safe]() macro, because we don't hold the lock and we're not the only ones that may delete things" explaining _why_ it's open-coded.

I dunno. Maybe Al disagrees. I just don't like using the "simple helpers" and then changing subtly how they work by knowing their internals.

Something like this

 fs/dcache.c |   14 ++++++++++++--
 fs/super.c  |   51 +++++++++++++++++++++++++++++++++++++++++++++------

 2 files changed, 57 insertions(+), 8 deletions(-)

Quoted 6 lines · Nick Piggin

Something like this

 fs/dcache.c |   14 ++++++++++++--
 fs/super.c  |   51 +++++++++++++++++++++++++++++++++++++++++++++------

 2 files changed, 57 insertions(+), 8 deletions(-)

Ok, I have to admit that I didn't expect it to blow up quite that badly - I thought it would add a line or two, not 50.

So wow I wonder if we should use your old approach instead, just make it an 'official' thing. IOW, maybe we can use "list_for_each_entry_safe()" after all, but simply introduce a "reset_next_entry()" helper or something and make that be part of the "calling convention" for those things.

But I can live with the open-coded version too. It just is a bit more code than I thought it would be.

Anybody? I don't really have very strong opinions.

In fairness I added a lot more comments and a bit of whitespace, accounting for about half of those lines added.

I wouldn't mind a list macro to reset. I'd prefer the name match better with the iterator macro though. list_safe_reset_next()?

Quoted 3 lines · Nick Piggin

In fairness I added a lot more comments and a bit of whitespace, accounting for about half of those lines added.

Yeah, ok. As mentioned, I can certainly go either way.

Quoted 3 lines

I wouldn't mind a list macro to reset. I'd prefer the name match better with the iterator macro though. list_safe_reset_next()?

Yeah, it needs to be at least "list_" something. So "list_safe_reset_next()" sounds as good as anything else. I wouldn't expect it to be very common.