| Message ID | 1334599360-15346-1-git-send-email-apw@canonical.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Apr 16, 2012 at 07:02:30PM +0100, Andy Whitcroft wrote: > We have been seeing a large number of reports of oopses in > ticket_spin_lock, these are mostly occuring in inotify related calls. > There seems to be several races in this code and there is a fairly > substantial patch set upstream to reorder the locking and sort this out. > I have pulled these patches back to precise from the notify upstream tree, > the patches themselves are slated for the 3.5 merge window. > > Patches follow, pull request below for those who prefer to look at them > locally. > > I am proposing them (somewhat reluctantly due to their size) for Precise. > > Comments? Just a comment: I do believe that this patch should also close LP#951537. I never marked that as a duplicate of LP#922906, but it looks like the same issue, with the racy fsnotify invocations. > -apw > > The following changes since commit 48853edab3b414cc73c3e1c037f477524053cf15: > > fsnotify: change locking order (2012-04-16 18:33:10 +0100) > > are available in the git repository at: > > git://kernel.ubuntu.com/apw/ubuntu-precise.git lp922906 > > for you to fetch changes up to 48853edab3b414cc73c3e1c037f477524053cf15: > > fsnotify: change locking order (2012-04-16 18:33:10 +0100) > > ---------------------------------------------------------------- > > Lino Sanfilippo (10): > inotify, fanotify: replace fsnotify_put_group() with > fsnotify_destroy_group() > fsnotify: introduce fsnotify_get_group() > fsnotify: use reference counting for groups > fsnotify: take groups mark_lock before mark lock > fanotify: add an extra flag to mark_remove_from_mask that indicates > wheather a mark should be destroyed > fsnotify: use a mutex instead of a spinlock to protect a groups mark > list > fsnotify: pass group to fsnotify_destroy_mark() > fsnotify: introduce locked versions of fsnotify_add_mark() and > fsnotify_remove_mark() > fsnotify: dont put marks on temporary list when clearing marks by > group > fsnotify: change locking order > > fs/notify/dnotify/dnotify.c | 4 +- > fs/notify/fanotify/fanotify_user.c | 33 +++++++----- > fs/notify/group.c | 40 +++++++-------- > fs/notify/inode_mark.c | 14 ++++-- > fs/notify/inotify/inotify_fsnotify.c | 4 +- > fs/notify/inotify/inotify_user.c | 11 ++-- > fs/notify/mark.c | 91 +++++++++++++++++++--------------- > fs/notify/vfsmount_mark.c | 14 ++++-- > include/linux/fsnotify_backend.h | 26 ++++++---- > kernel/audit_tree.c | 10 ++-- > kernel/audit_watch.c | 4 +- > 11 files changed, 147 insertions(+), 104 deletions(-) > > -- > 1.7.9.5 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team Cheers, -- Luis
On 04/16/2012 11:02 AM, Andy Whitcroft wrote: > We have been seeing a large number of reports of oopses in > ticket_spin_lock, these are mostly occuring in inotify related calls. > There seems to be several races in this code and there is a fairly > substantial patch set upstream to reorder the locking and sort this out. > I have pulled these patches back to precise from the notify upstream tree, > the patches themselves are slated for the 3.5 merge window. > > Patches follow, pull request below for those who prefer to look at them > locally. > > I am proposing them (somewhat reluctantly due to their size) for Precise. > > Comments? > > -apw > > The following changes since commit 48853edab3b414cc73c3e1c037f477524053cf15: > > fsnotify: change locking order (2012-04-16 18:33:10 +0100) > > are available in the git repository at: > > git://kernel.ubuntu.com/apw/ubuntu-precise.git lp922906 > > for you to fetch changes up to 48853edab3b414cc73c3e1c037f477524053cf15: > > fsnotify: change locking order (2012-04-16 18:33:10 +0100) > > ---------------------------------------------------------------- > > Lino Sanfilippo (10): > inotify, fanotify: replace fsnotify_put_group() with > fsnotify_destroy_group() > fsnotify: introduce fsnotify_get_group() > fsnotify: use reference counting for groups > fsnotify: take groups mark_lock before mark lock > fanotify: add an extra flag to mark_remove_from_mask that indicates > wheather a mark should be destroyed > fsnotify: use a mutex instead of a spinlock to protect a groups mark > list > fsnotify: pass group to fsnotify_destroy_mark() > fsnotify: introduce locked versions of fsnotify_add_mark() and > fsnotify_remove_mark() > fsnotify: dont put marks on temporary list when clearing marks by > group > fsnotify: change locking order > > fs/notify/dnotify/dnotify.c | 4 +- > fs/notify/fanotify/fanotify_user.c | 33 +++++++----- > fs/notify/group.c | 40 +++++++-------- > fs/notify/inode_mark.c | 14 ++++-- > fs/notify/inotify/inotify_fsnotify.c | 4 +- > fs/notify/inotify/inotify_user.c | 11 ++-- > fs/notify/mark.c | 91 +++++++++++++++++++--------------- > fs/notify/vfsmount_mark.c | 14 ++++-- > include/linux/fsnotify_backend.h | 26 ++++++---- > kernel/audit_tree.c | 10 ++-- > kernel/audit_watch.c | 4 +- > 11 files changed, 147 insertions(+), 104 deletions(-) > I've looked these over and I don't see any obvious problems. Are we proposing to take these before they are in either Linus' or the upstream stable trees? Problems I have taking them now: 1. Obviously they have not have the kind of upstream review that we require for most patches. 2. The patches won't have received a great deal of testing from upstream. However, people should recognise that most of these patches don't get any kind of real testing until they are picked up by a distro. 3. The number of changes and the type of changes (locking and reference counting). I'm supposed to be the one that guards the stability of the kernels (especially LTS kernels) and be more conservative that most. However, I'm inclined to take these patches, get them out and tested as best we can and as early in the support period that we can so that if there are issues, they are found and fixed as quickly as possible. Brad
On 16.04.2012 20:02, Andy Whitcroft wrote: > We have been seeing a large number of reports of oopses in > ticket_spin_lock, these are mostly occuring in inotify related calls. > There seems to be several races in this code and there is a fairly > substantial patch set upstream to reorder the locking and sort this out. > I have pulled these patches back to precise from the notify upstream tree, > the patches themselves are slated for the 3.5 merge window. > > Patches follow, pull request below for those who prefer to look at them > locally. > > I am proposing them (somewhat reluctantly due to their size) for Precise. > > Comments? > > -apw > > The following changes since commit 48853edab3b414cc73c3e1c037f477524053cf15: > > fsnotify: change locking order (2012-04-16 18:33:10 +0100) > > are available in the git repository at: > > git://kernel.ubuntu.com/apw/ubuntu-precise.git lp922906 > > for you to fetch changes up to 48853edab3b414cc73c3e1c037f477524053cf15: > > fsnotify: change locking order (2012-04-16 18:33:10 +0100) > > ---------------------------------------------------------------- > > Lino Sanfilippo (10): > inotify, fanotify: replace fsnotify_put_group() with > fsnotify_destroy_group() > fsnotify: introduce fsnotify_get_group() > fsnotify: use reference counting for groups > fsnotify: take groups mark_lock before mark lock > fanotify: add an extra flag to mark_remove_from_mask that indicates > wheather a mark should be destroyed > fsnotify: use a mutex instead of a spinlock to protect a groups mark > list > fsnotify: pass group to fsnotify_destroy_mark() > fsnotify: introduce locked versions of fsnotify_add_mark() and > fsnotify_remove_mark() > fsnotify: dont put marks on temporary list when clearing marks by > group > fsnotify: change locking order > > fs/notify/dnotify/dnotify.c | 4 +- > fs/notify/fanotify/fanotify_user.c | 33 +++++++----- > fs/notify/group.c | 40 +++++++-------- > fs/notify/inode_mark.c | 14 ++++-- > fs/notify/inotify/inotify_fsnotify.c | 4 +- > fs/notify/inotify/inotify_user.c | 11 ++-- > fs/notify/mark.c | 91 +++++++++++++++++++--------------- > fs/notify/vfsmount_mark.c | 14 ++++-- > include/linux/fsnotify_backend.h | 26 ++++++---- > kernel/audit_tree.c | 10 ++-- > kernel/audit_watch.c | 4 +- > 11 files changed, 147 insertions(+), 104 deletions(-) > There is as always two sides: the rules of stable sanity would rather say we should wait till the whole series is upstream and has a certain time to settle before considering this. Looking through the patchset which is large and touches mainly reference counts and locking (the natural sources of madness), there is still a certain feeling of nervousness about 3 and 8+9. It is hard to decide those things are safe or not without knowing the whole code by heart. On the other hand the locking of inotify seems to be broken a lot based on the many bug reports and duplicates. Or at least the problems are quite likely to be hit by a lot of people based on a common workflow. So it would be good to fix things sooner than later. If we do that, then I agree with Brad, better early. And maybe trying to be prepared with a revert-this emergency upload in the sleve... For some reason people always find the serious problems _after_ moving into updates... -Stefan
We have been seeing a large number of reports of oopses in ticket_spin_lock, these are mostly occuring in inotify related calls. There seems to be several races in this code and there is a fairly substantial patch set upstream to reorder the locking and sort this out. I have pulled these patches back to precise from the notify upstream tree, the patches themselves are slated for the 3.5 merge window. Patches follow, pull request below for those who prefer to look at them locally. I am proposing them (somewhat reluctantly due to their size) for Precise. Comments? -apw The following changes since commit 48853edab3b414cc73c3e1c037f477524053cf15: fsnotify: change locking order (2012-04-16 18:33:10 +0100) are available in the git repository at: git://kernel.ubuntu.com/apw/ubuntu-precise.git lp922906 for you to fetch changes up to 48853edab3b414cc73c3e1c037f477524053cf15: fsnotify: change locking order (2012-04-16 18:33:10 +0100) ---------------------------------------------------------------- Lino Sanfilippo (10): inotify, fanotify: replace fsnotify_put_group() with fsnotify_destroy_group() fsnotify: introduce fsnotify_get_group() fsnotify: use reference counting for groups fsnotify: take groups mark_lock before mark lock fanotify: add an extra flag to mark_remove_from_mask that indicates wheather a mark should be destroyed fsnotify: use a mutex instead of a spinlock to protect a groups mark list fsnotify: pass group to fsnotify_destroy_mark() fsnotify: introduce locked versions of fsnotify_add_mark() and fsnotify_remove_mark() fsnotify: dont put marks on temporary list when clearing marks by group fsnotify: change locking order fs/notify/dnotify/dnotify.c | 4 +- fs/notify/fanotify/fanotify_user.c | 33 +++++++----- fs/notify/group.c | 40 +++++++-------- fs/notify/inode_mark.c | 14 ++++-- fs/notify/inotify/inotify_fsnotify.c | 4 +- fs/notify/inotify/inotify_user.c | 11 ++-- fs/notify/mark.c | 91 +++++++++++++++++++--------------- fs/notify/vfsmount_mark.c | 14 ++++-- include/linux/fsnotify_backend.h | 26 ++++++---- kernel/audit_tree.c | 10 ++-- kernel/audit_watch.c | 4 +- 11 files changed, 147 insertions(+), 104 deletions(-)