locking problems: Btrfs: be more selective in the defrag ioctl

linux-btrfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* locking problems: Btrfs: be more selective in the defrag ioctl
@ 2010-03-20 11:38 Dan Carpenter
  2010-03-22 13:47 ` Josef Bacik
  0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2010-03-20 11:38 UTC (permalink / raw)
  To: chris.mason; +Cc: linux-btrfs, linux-kernel

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.

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: locking problems: Btrfs: be more selective in the defrag ioctl
  2010-03-20 11:38 locking problems: Btrfs: be more selective in the defrag ioctl Dan Carpenter
@ 2010-03-22 13:47 ` Josef Bacik
  2010-03-22 14:03   ` Dan Carpenter
  0 siblings, 1 reply; 4+ messages in thread
From: Josef Bacik @ 2010-03-22 13:47 UTC (permalink / raw)
  To: Dan Carpenter, chris.mason, linux-btrfs, linux-kernel

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,

Josef

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: locking problems: Btrfs: be more selective in the defrag ioctl
  2010-03-22 13:47 ` Josef Bacik
@ 2010-03-22 14:03   ` Dan Carpenter
  2010-03-22 14:09     ` Josef Bacik
  0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2010-03-22 14:03 UTC (permalink / raw)
  To: Josef Bacik; +Cc: chris.mason, linux-btrfs, linux-kernel

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: locking problems: Btrfs: be more selective in the defrag ioctl
  2010-03-22 14:03   ` Dan Carpenter
@ 2010-03-22 14:09     ` Josef Bacik
  0 siblings, 0 replies; 4+ messages in thread
From: Josef Bacik @ 2010-03-22 14:09 UTC (permalink / raw)
  To: Dan Carpenter, Josef Bacik, chris.mason, linux-btrfs,
	linux-kernel

On Mon, Mar 22, 2010 at 05:03:30PM +0300, Dan Carpenter wrote:
> 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.
> 

Ahh yeah you are right, should probably just put a mutex_unlock before the break
in both cases.  Thanks,

Josef

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2010-03-22 14:09 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-03-20 11:38 locking problems: Btrfs: be more selective in the defrag ioctl Dan Carpenter
2010-03-22 13:47 ` Josef Bacik
2010-03-22 14:03   ` Dan Carpenter
2010-03-22 14:09     ` Josef Bacik

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).