From: Josef Bacik <josef@toxicpanda.com>
To: Christoph Hellwig <hch@lst.de>
Cc: linux-btrfs@vger.kernel.org, kernel-team@fb.com
Subject: Re: [PATCH] btrfs: update device path inode time instead of bd_inode
Date: Thu, 14 Oct 2021 12:32:22 -0400 [thread overview]
Message-ID: <YWhblpJgxJ81nA5i@localhost.localdomain> (raw)
In-Reply-To: <20211014160520.GA445@lst.de>
On Thu, Oct 14, 2021 at 06:05:20PM +0200, Christoph Hellwig wrote:
> On Thu, Oct 14, 2021 at 12:00:10PM -0400, Josef Bacik wrote:
> > 1 fs/btrfs/volumes.c update_dev_time 1900 generic_update_time(d_inode(path.dentry), &now, S_MTIME | S_CTIME);
>
> This is the onde we're talking about.
>
> > 4 fs/inode.c inode_update_time 1789 return generic_update_time(inode, time, flags);
>
> This is update_time().
>
> And all others are ->update_time instances.
>
> > > Looking at this a bit more I think the right fix is to simply revert the
> > > offending commit. The lockdep complains was due to changes issues in the
> > > loop driver and has been fixed in the loop driver in the meantime.
> > >
> >
> > Where were they fixed? And it doesn't fix the fact that we're calling open on a
> > device, so any change at all to the loop device is going to end us back up in
> > this spot because we end up with the ->lo_mutex in our dependency chain. I want
> > to avoid this by not calling open, and that means looking up the inode and doing
> > operations without needing to go through the full file open path.
>
> This all looks like the open_mutex (formerly bd_mutex) vs lo_mutex inside
> and outside chains, and they were fixed.
>
They weren't, I just ran with the original fix reverted and I got the same
splat. We need to not call into the blockdevice open call at all, and even if
they were fixed we'd just get screwed at some point in the future. The less I
have to worry about things outside of fs/btrfs the better.
> > The best thing for btrfs here is to export the update_time() helper and call
> > that to avoid all the baggage that comes from opening the block device. Thanks,
>
> update_time is a bit too low-level for an export as it requires a fair
> effort to call it the right way.
It's about the same complexity as touch_atime(), and clearly I still need it to
fix the lockdep splat. Thanks,
Josef
prev parent reply other threads:[~2021-10-14 16:32 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-14 15:17 [PATCH] btrfs: update device path inode time instead of bd_inode Josef Bacik
2021-10-14 15:27 ` David Sterba
2021-10-14 15:33 ` Christoph Hellwig
2021-10-14 15:50 ` Josef Bacik
2021-10-14 15:53 ` Christoph Hellwig
2021-10-14 16:00 ` Josef Bacik
2021-10-14 16:05 ` Christoph Hellwig
2021-10-14 16:32 ` Josef Bacik [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YWhblpJgxJ81nA5i@localhost.localdomain \
--to=josef@toxicpanda.com \
--cc=hch@lst.de \
--cc=kernel-team@fb.com \
--cc=linux-btrfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox