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

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

[Josef Bacik]

Christoph pointed out that I'm updating bdev->bd_inode for the device
time when we remove block devices from a btrfs file system, however this
isn't actually exposed to anything.  The inode we want to update is the
one that's associated with the path to the device, usually on devtmpfs,
so that blkid notices the difference.

We still don't want to do the blkdev_open, so use kern_path() to get the
path to the given device and do the update time on that inode.

Fixes: 8f96a5bfa150 ("btrfs: update the bdev time directly when closing")
Reported-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/volumes.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 6031e2f4c6bc..c460f76b7033 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -14,6 +14,7 @@
 #include <linux/semaphore.h>
 #include <linux/uuid.h>
 #include <linux/list_sort.h>
+#include <linux/namei.h>
 #include "misc.h"
 #include "ctree.h"
 #include "extent_map.h"
@@ -1882,18 +1883,22 @@ static int btrfs_add_dev_item(struct btrfs_trans_handle *trans,
 /*
  * Function to update ctime/mtime for a given device path.
  * Mainly used for ctime/mtime based probe like libblkid.
+ *
+ * We don't care about errors here, this is just to be kind to userspace.
  */
-static void update_dev_time(struct block_device *bdev)
+static void update_dev_time(const char *device_path)
 {
-	struct inode *inode = bdev->bd_inode;
+	struct path path;
 	struct timespec64 now;
+	int ret;
 
-	/* Shouldn't happen but just in case. */
-	if (!inode)
+	ret = kern_path(device_path, LOOKUP_FOLLOW, &path);
+	if (ret)
 		return;
 
-	now = current_time(inode);
-	generic_update_time(inode, &now, S_MTIME | S_CTIME);
+	now = current_time(d_inode(path.dentry));
+	generic_update_time(d_inode(path.dentry), &now, S_MTIME | S_CTIME);
+	path_put(&path);
 }
 
 static int btrfs_rm_dev_item(struct btrfs_device *device)
@@ -2069,7 +2074,7 @@ void btrfs_scratch_superblocks(struct btrfs_fs_info *fs_info,
 	btrfs_kobject_uevent(bdev, KOBJ_CHANGE);
 
 	/* Update ctime/mtime for device path for libblkid */
-	update_dev_time(bdev);
+	update_dev_time(device_path);
 }
 
 int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
@@ -2728,7 +2733,7 @@ int btrfs_init_new_device(struct btrfs_fs_info *fs_info, const char *device_path
 	btrfs_forget_devices(device_path);
 
 	/* Update ctime/mtime for blkid or udev */
-	update_dev_time(bdev);
+	update_dev_time(device_path);
 
 	return ret;
 
-- 
2.26.3

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

[David Sterba]

On Thu, Oct 14, 2021 at 11:17:08AM -0400, Josef Bacik wrote:
> Christoph pointed out that I'm updating bdev->bd_inode for the device
> time when we remove block devices from a btrfs file system, however this
> isn't actually exposed to anything.  The inode we want to update is the
> one that's associated with the path to the device, usually on devtmpfs,
> so that blkid notices the difference.
> 
> We still don't want to do the blkdev_open, so use kern_path() to get the
> path to the given device and do the update time on that inode.
> 
> Fixes: 8f96a5bfa150 ("btrfs: update the bdev time directly when closing")
> Reported-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Added to misc-next, thanks.

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

[Christoph Hellwig]

On Thu, Oct 14, 2021 at 11:17:08AM -0400, Josef Bacik wrote:
> +	now = current_time(d_inode(path.dentry));
> +	generic_update_time(d_inode(path.dentry), &now, S_MTIME | S_CTIME);

This is still broken as it won't call into ->update_time.
generic_update_time is a helper/default for ->update_time, not something
for an external caller.

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

[Josef Bacik]

On Thu, Oct 14, 2021 at 05:33:47PM +0200, Christoph Hellwig wrote:
> On Thu, Oct 14, 2021 at 11:17:08AM -0400, Josef Bacik wrote:
> > +	now = current_time(d_inode(path.dentry));
> > +	generic_update_time(d_inode(path.dentry), &now, S_MTIME | S_CTIME);
> 
> This is still broken as it won't call into ->update_time.
> generic_update_time is a helper/default for ->update_time, not something
> for an external caller.

Then we probably need to fix all the people currently calling it.  In the
meantime I'll add a helper to use the thing that calls ->update_time instead.
Thanks,

Josef

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

[Christoph Hellwig]

On Thu, Oct 14, 2021 at 11:50:07AM -0400, Josef Bacik wrote:
> On Thu, Oct 14, 2021 at 05:33:47PM +0200, Christoph Hellwig wrote:
> > On Thu, Oct 14, 2021 at 11:17:08AM -0400, Josef Bacik wrote:
> > > +	now = current_time(d_inode(path.dentry));
> > > +	generic_update_time(d_inode(path.dentry), &now, S_MTIME | S_CTIME);
> > 
> > This is still broken as it won't call into ->update_time.
> > generic_update_time is a helper/default for ->update_time, not something
> > for an external caller.
> 
> Then we probably need to fix all the people currently calling it.

All other callers are from update_time / ->update_time.

> In the
> meantime I'll add a helper to use the thing that calls ->update_time instead.
> Thanks,

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.

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

[Josef Bacik]

On Thu, Oct 14, 2021 at 05:53:41PM +0200, Christoph Hellwig wrote:
> On Thu, Oct 14, 2021 at 11:50:07AM -0400, Josef Bacik wrote:
> > On Thu, Oct 14, 2021 at 05:33:47PM +0200, Christoph Hellwig wrote:
> > > On Thu, Oct 14, 2021 at 11:17:08AM -0400, Josef Bacik wrote:
> > > > +	now = current_time(d_inode(path.dentry));
> > > > +	generic_update_time(d_inode(path.dentry), &now, S_MTIME | S_CTIME);
> > > 
> > > This is still broken as it won't call into ->update_time.
> > > generic_update_time is a helper/default for ->update_time, not something
> > > for an external caller.
> > 
> > Then we probably need to fix all the people currently calling it.
> 
> All other callers are from update_time / ->update_time.
> 

I'm seeing a lot more calls to generic_update_time elsewhere

  File                Function             Line
0 fs/inode.c          <global>             1779 EXPORT_SYMBOL(generic_update_time);
1 fs/btrfs/volumes.c  update_dev_time      1900 generic_update_time(d_inode(path.dentry), &now, S_MTIME | S_CTIME);
2 fs/gfs2/inode.c     gfs2_update_time     2146 return generic_update_time(inode, time, flags);
3 fs/inode.c          generic_update_time  1755 int generic_update_time(struct inode *inode, struct timespec64 *time, int flags)
4 fs/inode.c          inode_update_time    1789 return generic_update_time(inode, time, flags);
5 fs/orangefs/inode.c orangefs_update_time  913 generic_update_time(inode, time, flags);
6 fs/ubifs/file.c     ubifs_update_time    1382 return generic_update_time(inode, time, flags);
7 fs/xfs/xfs_iops.c   xfs_vn_update_time   1123 return generic_update_time(inode, now, flags);
8 include/linux/fs.h  __printf             2617 extern int generic_update_time(struct inode *, struct timespec64 *, int );

> > In the
> > meantime I'll add a helper to use the thing that calls ->update_time instead.
> > Thanks,
> 
> 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.

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,

Josef

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

[Christoph Hellwig]

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.

> 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.

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

[Josef Bacik]

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