Messages in this thread | | | Date | Mon, 22 Mar 2010 17:03:30 +0300 | From | Dan Carpenter <> | Subject | Re: locking problems: Btrfs: be more selective in the defrag ioctl |
| |
On Mon, Mar 22, 2010 at 09:47:21AM -0400, Josef Bacik wrote: > On Sat, Mar 20, 2010 at 02:38:51PM +0300, Dan Carpenter wrote: > > Hi Chris, > > > > There is a locking problem in > > 940100a4a7b78 "Btrfs: be more selective in the defrag ioctl" > > > > There are two places where we break out of the while loop under the > > lock. > > > > fs/btrfs/ioctl.c +708 btrfs_defrag_file(159) error: double lock 'mutex:&inode->i_mutex' > > 600 mutex_lock(&inode->i_mutex); > > 601 if (range->flags & BTRFS_DEFRAG_RANGE_COMPRESS) > > 602 BTRFS_I(inode)->force_compress = 1; > > 603 > > 604 ret = btrfs_check_data_free_space(root, inode, PAGE_CACHE_SIZE); > > 605 if (ret) { > > 606 ret = -ENOSPC; > > 607 break; > > > > Here. > > > > 608 } > > 609 > > 610 ret = btrfs_reserve_metadata_for_delalloc(root, inode, 1); > > 611 if (ret) { > > 612 btrfs_free_reserved_data_space(root, inode, > > 613 PAGE_CACHE_SIZE); > > 614 ret = -ENOSPC; > > 615 break; > > > > And here. > > > > 616 } > > > > Maybe we should have "goto err_reservations;" instead of break? I > > don't know the code well enough to say. > > No, everything is accounted for correctly. If the metadata reservation fails, > we free the data space reservation and break. If the data space reservation > fails, we're good to go and can just exit. Thanks, >
What about the lock on line 606?
> > 600 mutex_lock(&inode->i_mutex);
If we break on line 615 or 607 that means that we return with the lock held, or if (range->flags & BTRFS_DEFRAG_RANGE_COMPRESS) is true then we dead lock.
regards, dan carpenter
> Josef
| |