Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 15 Sep 2021 12:31:40 +0200
From:   Jan Kara <jack@suse.cz>
To:     Amir Goldstein <amir73il@gmail.com>
Cc:     Gabriel Krisman Bertazi <krisman@collabora.com>,
        Jan Kara <jack@suse.cz>, Jan Kara <jack@suse.com>,
        Linux API <linux-api@vger.kernel.org>,
        Ext4 <linux-ext4@vger.kernel.org>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        Khazhismel Kumykov <khazhy@google.com>,
        David Howells <dhowells@redhat.com>,
        Dave Chinner <david@fromorbit.com>,
        Theodore Tso <tytso@mit.edu>,
        "Darrick J. Wong" <djwong@kernel.org>,
        Matthew Bobrowski <repnop@google.com>, kernel@collabora.com
Subject: Re: [PATCH v6 15/21] fanotify: Preallocate per superblock mark error
 event
Message-ID: <20210915103140.GA6166@quack2.suse.cz>
References: <20210812214010.3197279-1-krisman@collabora.com>
 <20210812214010.3197279-16-krisman@collabora.com>
 <20210816155758.GF30215@quack2.suse.cz>
 <877dg6rbtn.fsf@collabora.com>
 <87a6kusmar.fsf@collabora.com>
 <CAOQ4uxjDtA45nn4iT9LFbbavuGa=vMPQJFp7GOJHdqrst8y+1A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAOQ4uxjDtA45nn4iT9LFbbavuGa=vMPQJFp7GOJHdqrst8y+1A@mail.gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On Fri 03-09-21 07:16:33, Amir Goldstein wrote:
> On Fri, Sep 3, 2021 at 12:24 AM Gabriel Krisman Bertazi
> <krisman@collabora.com> wrote:
> > Actually, I don't think this will work for insertion unless we keep a
> > bounce buffer for the file_handle, because we need to keep the
> > group->notification_lock to ensure the fee doesn't go away with the mark
> > (since it is not yet enqueued) but, as discussed before, we don't want
> > to hold that lock when generating the FH.
> >
> > I think the correct way is to have some sort of refcount of the error
> > event slot.  We could use err_count for that and change the suggestion
> > above to:
> >
> > if (mark->flags & FANOTIFY_MARK_FLAG_SB_MARK) {
> >         struct fanotify_sb_mark *fa_mark = FANOTIFY_SB_MARK(mark);
> >
> >         spin_lock(&group->notification_lock);
> >         if (fa_mark->fee_slot) {
> >                 if (!fee->err_count) {
> >                         kfree(fa_mark->fee_slot);
> >                         fa_mark->fee_slot = NULL;
> >                 } else {
> >                         fa_mark->fee_slot->mark_alive = 0;
> >                 }
> >         }
> >         spin_unlock(&group->notification_lock);
> > }
> >
> > And insertion would look like this:
> >
> > static int fanotify_handle_error_event(....) {
> >
> >         spin_lock(&group->notification_lock);
> >
> >         if (!mark->fee || (mark->fee->err_count++) {
> >                 spin_unlock(&group->notification_lock);
> >                 return 0;
> >         }
> >
> >         spin_unlock(&group->notification_lock);
> >
> >         mark->fee->fae.type = FANOTIFY_EVENT_TYPE_FS_ERROR;
> >
> >         /* ... Write report data to error event ... */
> >
> >         fanotify_encode_fh(&fee->object_fh, fanotify_encode_fh_len(inode),
> >                            NULL, 0);
> >
> >         fsnotify_add_event(group, &fee->fae.fse, NULL);
> >    }
> >
> > Unless you think this is too hack-ish.
> >
> > To be fair, I think it is hack-ish.
> 
> Actually, I wouldn't mind the hack-ish-ness if it would simplify things,
> but I do not see how this is the case here.
> I still cannot wrap my head around the semantics, which is a big red light.
> First of all a suggestion should start with the lifetime rules:
> - Possible states
> - State transition rules
> 
> Speaking for myself, I simply cannot review a proposal without these
> documented rules.

Hum, getting back up to speed on this after vacation is tough which
suggests maybe we've indeed overengineered this :) So let's try to simplify
things.

> > I would add a proper refcount_t
> > to the error event, and let the mark own a reference to it, which is
> > dropped when the mark goes away.  Enqueue and Dequeue will acquire and
> > drop references, respectively. In this case, err_count is not
> > overloaded.
> >
> > Will it work?
> 
> Maybe, I still don't see the full picture, but if this can get us to a state
> where error events handling is simpler then it's a good idea.
> Saving the space of refcount_t in error event struct is not important at all.
> 
> But if Jan's option #1 (mempool) brings us to less special casing
> of enqueue/dequeue of error events, then I think that would be
> my preference.

Yes, I think mempools would result in a simpler code overall (the
complexity of recycling events would be handled by mempool for us). Maybe
we would not even need to play tricks with mempool resizing? We could just
make sure it has couple of events reserved and if it ever happens that
mempool_alloc() cannot give us any event, we'd report queue overflow (like
we already do for other event types if that happens). I think we could
require that callers generating error events are in a context where GFP_NOFS
allocation is OK - this should be achievable target for filesystems and
allocation failures should be rare with such mask.

									Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR