linux-ext4 - Re: [PATCH v7 2/9] ext4: for committing inode, make ext4_fc_track

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAD+ocbx0O88URnRZ6fQi6vAVxdH_gAk4G9eg9iz_OOeg-zT6mA@mail.gmail.com>
Date: Mon, 14 Apr 2025 09:57:45 -0700
From: harshad shirwadkar <harshadshirwadkar@...il.com>
To: Jan Kara <jack@...e.cz>
Cc: linux-ext4@...r.kernel.org, tytso@....edu, harshads@...gle.com
Subject: Re: [PATCH v7 2/9] ext4: for committing inode, make
 ext4_fc_track_inode wait

On Thu, Dec 12, 2024 at 2:00 PM Jan Kara <jack@...e.cz> wrote:
>
> On Sun 18-08-24 04:03:49, Harshad Shirwadkar wrote:
> > If the inode that's being requested to track using ext4_fc_track_inode
> > is being committed, then wait until the inode finishes the
> > commit. Also, add calls to ext4_fc_track_inode at the right places.
> >
> > With this patch, now calling ext4_reserve_inode_write() results in
> > inode being tracked for next fast commit. A subtle lock ordering
> > requirement with i_data_sem (which is documented in the code) requires
> > that ext4_fc_track_inode() be called before grabbing i_data_sem. So,
> > this patch also adds explicit ext4_fc_track_inode() calls in places
> > where i_data_sem grabbed.
> >
> > Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@...il.com>
>
> Sorry for the huge delay! Some comments are below:

Hi Jan,

Sorry for a huge delay from my end as well. Thank you for your
comments on V7 of this patchset. I just sent out a V8 of this patchset
where I have handled all of your comments on this and subsequent
patches in the series.

Thank you,
Harshad

>
> > @@ -598,6 +601,36 @@ void ext4_fc_track_inode(handle_t *handle, struct inode *inode)
> >       if (ext4_test_mount_flag(inode->i_sb, EXT4_MF_FC_INELIGIBLE))
> >               return;
> >
> > +     if (!list_empty(&ei->i_fc_list))
> > +             return;
> > +
> > +#ifdef CONFIG_LOCKDEP
> > +     /*
> > +      * If we come here, we may sleep while waiting for the inode to
> > +      * commit. We shouldn't be holding i_data_sem when we go to sleep since
> > +      * the commit path needs to grab the lock while committing the inode.
> > +      */
> > +     WARN_ON(lockdep_is_held(&ei->i_data_sem));
> > +#endif
>
> We have lockdep_assert_not_held() for this so you can avoid the ifdef.
>
> > +
> > +     while (ext4_test_inode_state(inode, EXT4_STATE_FC_COMMITTING)) {
> > +#if (BITS_PER_LONG < 64)
> > +             DEFINE_WAIT_BIT(wait, &ei->i_state_flags,
> > +                             EXT4_STATE_FC_COMMITTING);
> > +             wq = bit_waitqueue(&ei->i_state_flags,
> > +                                EXT4_STATE_FC_COMMITTING);
> > +#else
> > +             DEFINE_WAIT_BIT(wait, &ei->i_flags,
> > +                             EXT4_STATE_FC_COMMITTING);
> > +             wq = bit_waitqueue(&ei->i_flags,
> > +                                EXT4_STATE_FC_COMMITTING);
> > +#endif
> > +             prepare_to_wait(wq, &wait.wq_entry, TASK_UNINTERRUPTIBLE);
> > +             if (ext4_test_inode_state(inode, EXT4_STATE_FC_COMMITTING))
> > +                     schedule();
> > +             finish_wait(wq, &wait.wq_entry);
> > +     }
>
> But what protects us from fastcommit setting EXT4_STATE_FC_COMMITTING at
> this moment before we call ext4_fc_track_template(). Don't you need
> to grab sbi->s_fc_lock and hold it until the inode is attached to the
> fastcommit?
>
> I might be missing something so some documentation (like a comment here)
> would be nice to explain what are you actually trying to achieve with the
> waiting...
>
>                                                                 Honza
> --
> Jan Kara <jack@...e.com>
> SUSE Labs, CR