On Fri 29-06-18 16:20:12, Amir Goldstein wrote:
> On Thu, Jun 28, 2018 at 7:40 PM, Jan Kara <jack@xxxxxxx> wrote:
> > Audit tree code currently embeds fsnotify mark in audit_chunk. As chunk
> > attached to an inode is replace when new tag is added / removed, we also
> > need to remove old fsnotify mark and add a new one on such occasion.
> > This is cumbersome and makes locking rules somewhat difficult to follow.
> >
> > Fix these problems by allocating fsnotify mark independently and keeping
> > it all the time while there is some chunk attached to an inode. Also add
> > documentation about the locking rules so that things are easier to
> > follow.
> >
> > Signed-off-by: Jan Kara <jack@xxxxxxx>
> > ---
>
> Wow! this patch is a lot to take in at once.
> I wonder if it would be possible to split it to:
> - make mark not embedded and take chunk reference
> - replace_mark_chunk() and rid of cumbersome code
OK, I'll try something.
> Or any other simplification that would help me survive this review.
>
> In the mean while just one nit below...
>
> [...]
> > + mutex_lock(&entry->group->mark_mutex);
> > + spin_lock(&hash_lock);
> > + chunk = AUDIT_M(entry)->chunk;
> > + replace_mark_chunk(entry, NULL);
> > + spin_unlock(&hash_lock);
> > + if (chunk) {
> > + mutex_unlock(&entry->group->mark_mutex);
> > + evict_chunk(chunk);
> > + audit_mark_put_chunk(chunk);
> > + } else {
> > + mutex_unlock(&entry->group->mark_mutex);
> > + }
>
> else case seems like a leftover or something.
Fixed. Thanks.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
