On Fri, Oct 20, 2023 at 06:06:22PM +0300, Andy Shevchenko wrote:
> On Fri, Oct 20, 2023 at 05:51:59PM +0300, Andy Shevchenko wrote:
> > On Thu, Oct 19, 2023 at 11:43:47AM -0700, Linus Torvalds wrote:
> > > On Thu, 19 Oct 2023 at 11:16, Andy Shevchenko
> > > <andriy.shevchenko@xxxxxxxxx> wrote:
> > > >
> > > > Meanwhile a wild idea, can it be some git (automatic) conflict resolution that
> > > > makes that merge affect another (not related to the main contents of the merge)
> > > > files? Like upstream has one base, the merge has another which is older/newer
> > > > in the history?
> > >
> > > I already looked at any obvious case of that.
> > >
> > > The only quota-related issue on the other side is an obvious
> > > one-liner: commit 86be6b8bd834 ("quota: Check presence of quota
> > > operation structures instead of ->quota_read and ->quota_write
> > > callbacks").
> > >
> > > It didn't affect the merge, because it was not related to any of the
> > > changes that came in from the quota branch (it was physically close to
> > > the change that used lockdep_assert_held_write() instead of a
> > > WARN_ON_ONCE(down_read_trylock()) sequence, but it is unrelated to
> > > it).
> > >
> > > I guess you could try reverting that one-liner after the merge, but I
> > > _really_ don't think it is at all relevant.
> > >
> > > What *would* probably be interesting is to start at the pre-merge
> > > state, and rebase the code that got merged in. And then bisect *that*
> > > series.
> > >
> > > IOW, with the merge that triggers your bisection being commit
> > > 1500e7e0726e, do perhaps something like this:
> > >
> > > # Name the states before the merge
> > > git branch pre-merge 1500e7e0726e^
> > > git branch jan-state 1500e7e0726e^2
> > >
> > > # Now double-check that this works for you, of course.
> > > # Just to be safe...
> > > git checkout pre-merge
> > > .. test-build and test-boot this with the bad config ..
> >
> > That's I have checked already [4], but okay, let me double check.
> > [5] is the same as [4] according to `git diff`.
> >
> > It boots.
> >
> > > # Then, let's create a new branch that is
> > > # the rebased version of Jan's state:
> > > git checkout -b jan-rebased jan-state
> > > git rebase pre-merge
> >
> > [6] is created.
> >
> > > # Verify that the tree is the same as the merge
> > > git diff 1500e7e0726e
> >
> > Yes, nothing in output.
> >
> > And it does not boot.
> >
> > > # Ok, that was empty, so do a bisect on this
> > > # rebased history
> > > git bisect start
> > > git bisect bad
> > > git bisect good pre-merge
> > >
> > > .. and see what commit it *now* claims is the bad commit.
> >
> > git bisect start
> > # status: waiting for both good and bad commits
> > # good: [63580f669d7ff5aa5a1fa2e3994114770a491722] Merge tag 'ovl-update-6.6' of git://git.kernel.org/pub/scm/linux/kernel/git/overlayfs/vfs
> > git bisect good 63580f669d7ff5aa5a1fa2e3994114770a491722
> > # status: waiting for bad commit, 1 good commit known
> > # bad: [2447ff4196091e41d385635f9b6d003119f24199] ext2: Fix kernel-doc warnings
> > git bisect bad 2447ff4196091e41d385635f9b6d003119f24199
> > # bad: [a7c4109a1fa7f9f8cfa9aa93e7aae52d0df820f6] MAINTAINERS: change reiserfs status to obsolete
> > git bisect bad a7c4109a1fa7f9f8cfa9aa93e7aae52d0df820f6
> > # bad: [74fdc82e4a4302bf8a519101a40691b78d9beb6c] quota: add new helper dquot_active()
> > git bisect bad 74fdc82e4a4302bf8a519101a40691b78d9beb6c
> > # bad: [e64db1c50eb5d3be2187b56d32ec39e56b739845] quota: factor out dquot_write_dquot()
> > git bisect bad e64db1c50eb5d3be2187b56d32ec39e56b739845
> > # good: [eea7e964642984753768ddbb710e2eefd32c7a89] ext2: remove redundant assignment to variable desc and variable best_desc
> > git bisect good eea7e964642984753768ddbb710e2eefd32c7a89
> > # first bad commit: [e64db1c50eb5d3be2187b56d32ec39e56b739845] quota: factor out dquot_write_dquot()
> >
> > > Would you be willing to do this? It should be only a few bisects,
> > > since Jan's branch only brought in 17 commits that the above rebases
> > > into this test branch. So four or five bisections should pinpoint the
> > > exact point where it goes bad.
> >
> > See above.
> >
> > I even rebuilt again with just rebased on top of e64db1c50eb5 and it doesn't
> > boot, so we found the culprit that triggers this issue.
> >
> > > Of course, since this is apparently about some "random code generation
> > > issue", that exact point still may not be very interesting.
> >
> > On top of the above I have tried the following:
> > 1) dropping inline, replacing it to __always_inline -- no help;
> > 2) commenting out the error message -- helps!
> >
> > --- a/fs/quota/dquot.c
> > +++ b/fs/quota/dquot.c
> > @@ -632,8 +632,10 @@ static inline int dquot_write_dquot(struct dquot *dquot)
> > {
> > int ret = dquot->dq_sb->dq_op->write_dquot(dquot);
> > if (ret < 0) {
> > +#if 0
> > quota_error(dquot->dq_sb, "Can't write quota structure "
> > "(error %d). Quota may get out of sync!", ret);
> > +#endif
> > /* Clear dirty bit anyway to avoid infinite loop. */
> > clear_dquot_dirty(dquot);
> > }
Doing the same on the my branch based on top of v6.6-rc6 does not help.
So looks like a race condition somewhere happening related to that dirty bit
(as comment states it needs to be cleaned to avoid infinite loop, that's
probably what happens).
> > If it's a timing issue it's related to that error message, as the new helper is
> > run outside of the spinlock.
> >
> > What's is fishy there besides the error message being available only in one
> > case, is the pointer that is used for dp_op. I'm not at all familiar with the
> > code, but can it be that these superblocks are different for those two cases?
> >
> > [4]: https://bitbucket.org/andy-shev/linux/src/test-mrfld-before-merge/
> > [5]: https://bitbucket.org/andy-shev/linux/src/test-mrfld-pre-merge/
> > [6]: https://bitbucket.org/andy-shev/linux/src/test-mrfld-jan-rebased/
--
With Best Regards,
Andy Shevchenko
