Re: [PATCH] btrfs: update device path inode time instead of bd_inode

Josef Bacik
2021-10-14T16:32:28Z

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