[PATCH] make inotify event handles use GFP_NOFS

[PATCH] make inotify event handles use GFP_NOFS

[Eric Paris]

I think this is a bandaide to shut up lockdep.  I could either figure
out lockdep classes and figure out how to reclassify inotify locks since
I believe Nick is correct when he says inotify watches pin the inode in
core so memory pressure can't evict it.  I don't want to do that as I
think the real fix is my next generation fsnotify which does zero
allocations under locks and so everything can be GFP_KERNEL.  I'm
posting this as it is clearly safe and should fix the issue.

http://marc.info/?l=linux-kernel&m=123617147432377&w=2

includes a lockdep warning that shows while we are reclaiming FS memory
and inode may get evicted which generates an IN_IGNORED message.  Half
of that code path already used GFP_NOFS but a second allocation to store
the filename was using GFP_KERNEL.  As a precaution I also moved the
audit handle_event code path to use GFP_NOFS.

This is much the same as the precaution in f04b30de3c82528 which did
something similar.

Signed-off-by: Eric Paris <eparis@redhat.com>

---

 fs/notify/inotify/inotify_user.c |    2 +-
 kernel/auditfilter.c             |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)


diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index bed766e..1634319 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -220,7 +220,7 @@ static struct inotify_kernel_event * kernel_event(s32 wd, u32 mask, u32 cookie,
 				rem = 0;
 		}
 
-		kevent->name = kmalloc(len + rem, GFP_KERNEL);
+		kevent->name = kmalloc(len + rem, GFP_NOFS);
 		if (unlikely(!kevent->name)) {
 			kmem_cache_free(event_cachep, kevent);
 			return NULL;
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index fbf24d1..a3fa2c0 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -1028,7 +1028,7 @@ static void audit_update_watch(struct audit_parent *parent,
 
 		if (audit_enabled) {
 			struct audit_buffer *ab;
-			ab = audit_log_start(NULL, GFP_KERNEL,
+			ab = audit_log_start(NULL, GFP_NOFS,
 				AUDIT_CONFIG_CHANGE);
 			audit_log_format(ab, "auid=%u ses=%u",
 				audit_get_loginuid(current),
@@ -1067,7 +1067,7 @@ static void audit_remove_parent_watches(struct audit_parent *parent)
 			e = container_of(r, struct audit_entry, rule);
 			if (audit_enabled) {
 				struct audit_buffer *ab;
-				ab = audit_log_start(NULL, GFP_KERNEL,
+				ab = audit_log_start(NULL, GFP_NOFS,
 					AUDIT_CONFIG_CHANGE);
 				audit_log_format(ab, "auid=%u ses=%u",
 					audit_get_loginuid(current),

Re: [PATCH] make inotify event handles use GFP_NOFS

[Andrew Morton]

On Wed, 18 Mar 2009 14:27:32 -0400
Eric Paris <eparis@redhat.com> wrote:

> I think this is a bandaide to shut up lockdep.  I could either figure
> out lockdep classes and figure out how to reclassify inotify locks since
> I believe Nick is correct when he says inotify watches pin the inode in
> core so memory pressure can't evict it.

It's pretty sad to degrading the strength of the memory allocation just
to squish a lockdep report.

>  I don't want to do that as I
> think the real fix is my next generation fsnotify which does zero
> allocations under locks and so everything can be GFP_KERNEL.

I assume that's the 13-patch series further down in my todo pile.

Perhaps this workaround is suitable for 2.6.29.x, or 2.6.30 if the
13-patch-series was too late.  But do we care enough?


>  I'm
> posting this as it is clearly safe and should fix the issue.
> 
> http://marc.info/?l=linux-kernel&m=123617147432377&w=2
> 
> includes a lockdep warning that shows while we are reclaiming FS memory
> and inode may get evicted which generates an IN_IGNORED message.  Half
> of that code path already used GFP_NOFS but a second allocation to store
> the filename was using GFP_KERNEL.  As a precaution I also moved the
> audit handle_event code path to use GFP_NOFS.
> 
> This is much the same as the precaution in f04b30de3c82528 which did
> something similar.
> 

Re: [PATCH] make inotify event handles use GFP_NOFS

[Peter Zijlstra]

On Mon, 2009-03-30 at 14:26 -0700, Andrew Morton wrote:
> On Wed, 18 Mar 2009 14:27:32 -0400
> Eric Paris <eparis@redhat.com> wrote:
> 
> > I think this is a bandaide to shut up lockdep.  I could either figure
> > out lockdep classes and figure out how to reclassify inotify locks since
> > I believe Nick is correct when he says inotify watches pin the inode in
> > core so memory pressure can't evict it.
> 
> It's pretty sad to degrading the strength of the memory allocation just
> to squish a lockdep report.

Yeah, I agree, its the wrong thing to do. lockdep annotations really
aren't that hard -- also, you could also talk me through it.

Re: [PATCH] make inotify event handles use GFP_NOFS

[Peter Zijlstra]

On Mon, 2009-03-30 at 23:33 +0200, Peter Zijlstra wrote:
> On Mon, 2009-03-30 at 14:26 -0700, Andrew Morton wrote:
> > On Wed, 18 Mar 2009 14:27:32 -0400
> > Eric Paris <eparis@redhat.com> wrote:
> > 
> > > I think this is a bandaide to shut up lockdep.  I could either figure
> > > out lockdep classes and figure out how to reclassify inotify locks since
> > > I believe Nick is correct when he says inotify watches pin the inode in
> > > core so memory pressure can't evict it.
> > 
> > It's pretty sad to degrading the strength of the memory allocation just
> > to squish a lockdep report.
> 
> Yeah, I agree, its the wrong thing to do. lockdep annotations really
> aren't that hard -- also, you could also talk me through it.

static struct lock_class_key inotify_mutex_free;


     /*
      * here the inotify mutex gets moved to a different
      * data structure with different locking semantics while
      * holding inotify_mutex.
      */
     lock_set_class(&foo->inotify_mutex.dep_map, "inotify_mutex_free",
                     &inotify_mutex_free, 0, _THIS_IP_);


Or when done without holding the inotify_mutex


     /*
      * here the inotify mutex gets moved to a different
      * data structure with different locking semantics.
      */
     lockdep_set_class(&foo->inotify_mutex, &inotify_mutex_free);


Is all there should be to it.

Re: [PATCH] make inotify event handles use GFP_NOFS

[eparis]

On Apr 3, 2009, at 10:33 AM, Peter Zijlstra <peterz@infradead.org>  
wrote:

> On Mon, 2009-03-30 at 23:33 +0200, Peter Zijlstra wrote:
>> On Mon, 2009-03-30 at 14:26 -0700, Andrew Morton wrote:
>>> On Wed, 18 Mar 2009 14:27:32 -0400
>>> Eric Paris <eparis@redhat.com> wrote:
>>>
>>>> I think this is a bandaide to shut up lockdep.  I could either  
>>>> figure
>>>> out lockdep classes and figure out how to reclassify inotify  
>>>> locks since
>>>> I believe Nick is correct when he says inotify watches pin the  
>>>> inode in
>>>> core so memory pressure can't evict it.
>>>
>>> It's pretty sad to degrading the strength of the memory allocation  
>>> just
>>> to squish a lockdep report.
>>
>> Yeah, I agree, its the wrong thing to do. lockdep annotations really
>> aren't that hard -- also, you could also talk me through it.
>
> static struct lock_class_key inotify_mutex_free;
>
>
>     /*
>      * here the inotify mutex gets moved to a different
>      * data structure with different locking semantics while
>      * holding inotify_mutex.
>      */
>     lock_set_class(&foo->inotify_mutex.dep_map, "inotify_mutex_free",
>                     &inotify_mutex_free, 0, _THIS_IP_);
>
>
> Or when done without holding the inotify_mutex
>
>
>     /*
>      * here the inotify mutex gets moved to a different
>      * data structure with different locking semantics.
>      */
>     lockdep_set_class(&foo->inotify_mutex, &inotify_mutex_free);
>
>
> Is all there should be to it.

Thanks I'll try to send something soon.

-Eric