+Cc: Baokun, the author of the patch.
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);
> }
>
> 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
>
>
--
With Best Regards,
Andy Shevchenko
