[PATCH 0/2] xfs: regression fixes for 3.8 merge cycle

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/2] xfs: regression fixes for 3.8 merge cycle

Dave Chinner
Folks,

Here's a couple of fixes for regressions reported in the past couple
of days. They've passed xfstests smoke testing here. I'm not really
certain whether the direct Io deadlock fix is the best way to fix
the problem, but I can't think of any other way of doing it at this
point because we have no way of passing a transaction context to the
direct IO allocation callback path. If anyone has any bright ideas,
I'm all ears....

Cheers,

Dave.

_______________________________________________
xfs mailing list
[hidden email]
http://oss.sgi.com/mailman/listinfo/xfs
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/2] xfs: inode allocation should use unmapped buffers.

Dave Chinner
From: Dave Chinner <[hidden email]>

Inode buffers do not need to be mapped as inodes are read or written
directly from/to the pages underlying the buffer. This fixes a
regression introduced by commit 611c994 ("xfs: make XBF_MAPPED the
default behaviour").

Signed-off-by: Dave Chinner <[hidden email]>
---
 fs/xfs/xfs_ialloc.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_ialloc.c b/fs/xfs/xfs_ialloc.c
index 2d6495e..a815412 100644
--- a/fs/xfs/xfs_ialloc.c
+++ b/fs/xfs/xfs_ialloc.c
@@ -200,7 +200,8 @@ xfs_ialloc_inode_init(
  */
  d = XFS_AGB_TO_DADDR(mp, agno, agbno + (j * blks_per_cluster));
  fbuf = xfs_trans_get_buf(tp, mp->m_ddev_targp, d,
- mp->m_bsize * blks_per_cluster, 0);
+ mp->m_bsize * blks_per_cluster,
+ XBF_UNMAPPED);
  if (!fbuf)
  return ENOMEM;
  /*
--
1.7.10

_______________________________________________
xfs mailing list
[hidden email]
http://oss.sgi.com/mailman/listinfo/xfs
Reply | Threaded
Open this post in threaded view
|

[PATCH 2/2] xfs: fix direct IO nested transaction deadlock.

Dave Chinner
In reply to this post by Dave Chinner
From: Dave Chinner <[hidden email]>

The direct IO path can do a nested transaction reservation when
writing past the EOF. The first transaction is the append
transaction for setting the filesize at IO completion, but we can
also need a transaction for allocation of blocks. If the log is low
on space due to reservations and small log, the append transaction
can be granted after wating for space as the only active transaction
in the system. This then attempts a reservation for an allocation,
which there isn't space in the log for, and the reservation sleeps.
The result is that there is nothing left in the system to wake up
all the processes waiting for log space to come free.

The stack trace that shows this deadlock is relatively innocuous:

 xlog_grant_head_wait
 xlog_grant_head_check
 xfs_log_reserve
 xfs_trans_reserve
 xfs_iomap_write_direct
 __xfs_get_blocks
 xfs_get_blocks_direct
 do_blockdev_direct_IO
 __blockdev_direct_IO
 xfs_vm_direct_IO
 generic_file_direct_write
 xfs_file_dio_aio_writ
 xfs_file_aio_write
 do_sync_write
 vfs_write

This was discovered on a filesystem with a log of only 10MB, and a
log stripe unit of 256k whih increased the base reservations by
512k. Hence a allocation transaction requires 1.2MB of log space to
be available instead of only 260k, and so greatly increased the
chance that there wouldn't be enough log space available for the
nested transaction to succeed. The key to reproducing it is this
mkfs command:

mkfs.xfs -f -d agcount=16,su=256k,sw=12 -l su=256k,size=2560b $SCRATCH_DEV

The test case was a 1000 fsstress processes running with random
freeze and unfreezes every few seconds. Thanks to Eryu Guan
([hidden email]) for writing the test that found this on a system
with a somewhat unique default configuration....

cc: <[hidden email]>
Signed-off-by: Dave Chinner <[hidden email]>
---
 fs/xfs/xfs_aops.c |   78 +++++++++++++++++++----------------------------------
 fs/xfs/xfs_log.c  |    3 ++-
 2 files changed, 30 insertions(+), 51 deletions(-)

diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index 71361da..43c794e 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -151,9 +151,11 @@ xfs_setfilesize(
  /*
  * The transaction was allocated in the I/O submission thread,
  * thus we need to mark ourselves as beeing in a transaction
- * manually.
+ * manually. Similarly for freeze protection.
  */
  current_set_flags_nested(&tp->t_pflags, PF_FSTRANS);
+ rwsem_acquire_read(&VFS_I(ip)->i_sb->s_writers.lock_map[SB_FREEZE_FS-1],
+   0, 1, _THIS_IP_);
 
  xfs_ilock(ip, XFS_ILOCK_EXCL);
  isize = xfs_new_eof(ip, ioend->io_offset + ioend->io_size);
@@ -205,15 +207,6 @@ xfs_end_io(
  struct xfs_inode *ip = XFS_I(ioend->io_inode);
  int error = 0;
 
- if (ioend->io_append_trans) {
- /*
- * We've got freeze protection passed with the transaction.
- * Tell lockdep about it.
- */
- rwsem_acquire_read(
- &ioend->io_inode->i_sb->s_writers.lock_map[SB_FREEZE_FS-1],
- 0, 1, _THIS_IP_);
- }
  if (XFS_FORCED_SHUTDOWN(ip->i_mount)) {
  ioend->io_error = -EIO;
  goto done;
@@ -226,35 +219,33 @@ xfs_end_io(
  * range to normal written extens after the data I/O has finished.
  */
  if (ioend->io_type == XFS_IO_UNWRITTEN) {
- /*
- * For buffered I/O we never preallocate a transaction when
- * doing the unwritten extent conversion, but for direct I/O
- * we do not know if we are converting an unwritten extent
- * or not at the point where we preallocate the transaction.
- */
- if (ioend->io_append_trans) {
- ASSERT(ioend->io_isdirect);
-
- current_set_flags_nested(
- &ioend->io_append_trans->t_pflags, PF_FSTRANS);
- xfs_trans_cancel(ioend->io_append_trans, 0);
- }
-
  error = xfs_iomap_write_unwritten(ip, ioend->io_offset,
- ioend->io_size);
- if (error) {
- ioend->io_error = -error;
+  ioend->io_size);
+ goto done;
+ }
+
+ /*
+ * For direct I/O we do not know if we need to allocate blocks or not so
+ * we can't preallocate an append transaction as that results in nested
+ * reservations and log space deadlocks. Hence allocate the transaction
+ * here.  For buffered I/O we preallocate a transaction when submitting
+ * the IO.
+ */
+ if (ioend->io_isdirect && xfs_ioend_is_append(ioend)) {
+ error = xfs_setfilesize_trans_alloc(ioend);
+ if (error)
  goto done;
- }
- } else if (ioend->io_append_trans) {
+ }
+
+ if (ioend->io_append_trans) {
  error = xfs_setfilesize(ioend);
- if (error)
- ioend->io_error = -error;
  } else {
  ASSERT(!xfs_ioend_is_append(ioend));
  }
 
 done:
+ if (error)
+ ioend->io_error = -error;
  xfs_destroy_ioend(ioend);
 }
 
@@ -1432,25 +1423,21 @@ xfs_vm_direct_IO(
  size_t size = iov_length(iov, nr_segs);
 
  /*
- * We need to preallocate a transaction for a size update
- * here.  In the case that this write both updates the size
- * and converts at least on unwritten extent we will cancel
- * the still clean transaction after the I/O has finished.
+ * We cannot preallocate a size update transaction here as we
+ * don't know whether allocation is necessary or not. Hence we
+ * can only tell IO completion that one is necessary if we are
+ * not doing unwritten extent conversion.
  */
  iocb->private = ioend = xfs_alloc_ioend(inode, XFS_IO_DIRECT);
- if (offset + size > XFS_I(inode)->i_d.di_size) {
- ret = xfs_setfilesize_trans_alloc(ioend);
- if (ret)
- goto out_destroy_ioend;
+ if (offset + size > XFS_I(inode)->i_d.di_size)
  ioend->io_isdirect = 1;
- }
 
  ret = __blockdev_direct_IO(rw, iocb, inode, bdev, iov,
     offset, nr_segs,
     xfs_get_blocks_direct,
     xfs_end_io_direct_write, NULL, 0);
  if (ret != -EIOCBQUEUED && iocb->private)
- goto out_trans_cancel;
+ goto out_destroy_ioend;
  } else {
  ret = __blockdev_direct_IO(rw, iocb, inode, bdev, iov,
     offset, nr_segs,
@@ -1460,15 +1447,6 @@ xfs_vm_direct_IO(
 
  return ret;
 
-out_trans_cancel:
- if (ioend->io_append_trans) {
- current_set_flags_nested(&ioend->io_append_trans->t_pflags,
- PF_FSTRANS);
- rwsem_acquire_read(
- &inode->i_sb->s_writers.lock_map[SB_FREEZE_FS-1],
- 0, 1, _THIS_IP_);
- xfs_trans_cancel(ioend->io_append_trans, 0);
- }
 out_destroy_ioend:
  xfs_destroy_ioend(ioend);
  return ret;
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 1d6d2ee..aa734de 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -459,7 +459,8 @@ xfs_log_reserve(
  tic->t_trans_type = t_type;
  *ticp = tic;
 
- xlog_grant_push_ail(log, tic->t_unit_res * tic->t_cnt);
+ xlog_grant_push_ail(log, tic->t_cnt ? tic->t_unit_res * tic->t_cnt
+    : tic->t_unit_res);
 
  trace_xfs_log_reserve(log, tic);
 
--
1.7.10

_______________________________________________
xfs mailing list
[hidden email]
http://oss.sgi.com/mailman/listinfo/xfs
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] xfs: inode allocation should use unmapped buffers.

Christoph Hellwig
In reply to this post by Dave Chinner
On Tue, Nov 20, 2012 at 10:27:10PM +1100, Dave Chinner wrote:
> From: Dave Chinner <[hidden email]>
>
> Inode buffers do not need to be mapped as inodes are read or written
> directly from/to the pages underlying the buffer. This fixes a
> regression introduced by commit 611c994 ("xfs: make XBF_MAPPED the
> default behaviour").
>
> Signed-off-by: Dave Chinner <[hidden email]>

Looks good,

Reviewed-by: Christoph Hellwig <[hidden email]>

_______________________________________________
xfs mailing list
[hidden email]
http://oss.sgi.com/mailman/listinfo/xfs
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] xfs: fix direct IO nested transaction deadlock.

Christoph Hellwig
In reply to this post by Dave Chinner
On Tue, Nov 20, 2012 at 10:27:11PM +1100, Dave Chinner wrote:

> This was discovered on a filesystem with a log of only 10MB, and a
> log stripe unit of 256k whih increased the base reservations by
> 512k. Hence a allocation transaction requires 1.2MB of log space to
> be available instead of only 260k, and so greatly increased the
> chance that there wouldn't be enough log space available for the
> nested transaction to succeed. The key to reproducing it is this
> mkfs command:
>
> mkfs.xfs -f -d agcount=16,su=256k,sw=12 -l su=256k,size=2560b $SCRATCH_DEV
>
> The test case was a 1000 fsstress processes running with random
> freeze and unfreezes every few seconds. Thanks to Eryu Guan
> ([hidden email]) for writing the test that found this on a system
> with a somewhat unique default configuration....

That sounds like something that could fit xfstests fairly easily.

Re the patch - you're moving the transaction allocation back into the
end_io handler.  That's what my original version did, and I'm pretty
sure you talked me out of it back then.  I can't remember the details
but the list should have it.

> @@ -151,9 +151,11 @@ xfs_setfilesize(
>   /*
>   * The transaction was allocated in the I/O submission thread,
>   * thus we need to mark ourselves as beeing in a transaction
> - * manually.
> + * manually. Similarly for freeze protection.
>   */
>   current_set_flags_nested(&tp->t_pflags, PF_FSTRANS);
> + rwsem_acquire_read(&VFS_I(ip)->i_sb->s_writers.lock_map[SB_FREEZE_FS-1],
> +   0, 1, _THIS_IP_);

The comment above isn't true anymore, and the flags hack should be
removed.

I'm also not sure the freeze protection still works if the acquire is
outside the original broader scope protection.  I'll defer to Jan on
this as I don't really understand this magic enough.q
should be removed respectively replaced with sb_start_intwrite/sb_end_intwrite

>   if (ioend->io_type == XFS_IO_UNWRITTEN) {
>   error = xfs_iomap_write_unwritten(ip, ioend->io_offset,
> +  ioend->io_size);
> + goto done;
> + }
> +
> + /*
> + * For direct I/O we do not know if we need to allocate blocks or not so
> + * we can't preallocate an append transaction as that results in nested
> + * reservations and log space deadlocks. Hence allocate the transaction
> + * here.  For buffered I/O we preallocate a transaction when submitting
> + * the IO.
> + */
> + if (ioend->io_isdirect && xfs_ioend_is_append(ioend)) {

xfs_iomap_write_unwritten already updates the inode size, so this should
be an "else if"

> + error = xfs_setfilesize_trans_alloc(ioend);
> + if (error)
> + }
> +
> + if (ioend->io_append_trans) {
>   error = xfs_setfilesize(ioend);
>   } else {
>   ASSERT(!xfs_ioend_is_append(ioend));
>   }

Does it really make sense to have different allocation points for
buffered vs direct I/O? At least it needs a comment explaining why
it's done differently.

While it's probably too much work for a quick fix I'd much rather
replace the hacks we currently do in the direct I/O code with a
scheme where the dio code has a hook to allocate the transaction if
needed at the right point.

_______________________________________________
xfs mailing list
[hidden email]
http://oss.sgi.com/mailman/listinfo/xfs
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] xfs: fix direct IO nested transaction deadlock.

Jan Kara
On Tue 20-11-12 11:10:15, Christoph Hellwig wrote:

> On Tue, Nov 20, 2012 at 10:27:11PM +1100, Dave Chinner wrote:
> > This was discovered on a filesystem with a log of only 10MB, and a
> > log stripe unit of 256k whih increased the base reservations by
> > 512k. Hence a allocation transaction requires 1.2MB of log space to
> > be available instead of only 260k, and so greatly increased the
> > chance that there wouldn't be enough log space available for the
> > nested transaction to succeed. The key to reproducing it is this
> > mkfs command:
> >
> > mkfs.xfs -f -d agcount=16,su=256k,sw=12 -l su=256k,size=2560b $SCRATCH_DEV
> >
> > The test case was a 1000 fsstress processes running with random
> > freeze and unfreezes every few seconds. Thanks to Eryu Guan
> > ([hidden email]) for writing the test that found this on a system
> > with a somewhat unique default configuration....
>
> That sounds like something that could fit xfstests fairly easily.
>
> Re the patch - you're moving the transaction allocation back into the
> end_io handler.  That's what my original version did, and I'm pretty
> sure you talked me out of it back then.  I can't remember the details
> but the list should have it.
>
> > @@ -151,9 +151,11 @@ xfs_setfilesize(
> >   /*
> >   * The transaction was allocated in the I/O submission thread,
> >   * thus we need to mark ourselves as beeing in a transaction
> > - * manually.
> > + * manually. Similarly for freeze protection.
> >   */
> >   current_set_flags_nested(&tp->t_pflags, PF_FSTRANS);
> > + rwsem_acquire_read(&VFS_I(ip)->i_sb->s_writers.lock_map[SB_FREEZE_FS-1],
> > +   0, 1, _THIS_IP_);
>
> The comment above isn't true anymore, and the flags hack should be
> removed.
  It still seems to be true for buffered IO or am I misreading the code?

> I'm also not sure the freeze protection still works if the acquire is
> outside the original broader scope protection.  I'll defer to Jan on
> this as I don't really understand this magic enough.q
> should be removed respectively replaced with sb_start_intwrite/sb_end_intwrite
  It seems to work OK. If it were not for buffered IO path which allocates
a transaction (and thus freeze protection) in xfs_vm_writepage() we could
get rid of this lockdep magic. But so far we can't...

                                                                Honza
--
Jan Kara <[hidden email]>
SUSE Labs, CR

_______________________________________________
xfs mailing list
[hidden email]
http://oss.sgi.com/mailman/listinfo/xfs
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] xfs: fix direct IO nested transaction deadlock.

Dave Chinner
In reply to this post by Christoph Hellwig
On Tue, Nov 20, 2012 at 11:10:15AM -0500, Christoph Hellwig wrote:

> On Tue, Nov 20, 2012 at 10:27:11PM +1100, Dave Chinner wrote:
> > This was discovered on a filesystem with a log of only 10MB, and a
> > log stripe unit of 256k whih increased the base reservations by
> > 512k. Hence a allocation transaction requires 1.2MB of log space to
> > be available instead of only 260k, and so greatly increased the
> > chance that there wouldn't be enough log space available for the
> > nested transaction to succeed. The key to reproducing it is this
> > mkfs command:
> >
> > mkfs.xfs -f -d agcount=16,su=256k,sw=12 -l su=256k,size=2560b $SCRATCH_DEV
> >
> > The test case was a 1000 fsstress processes running with random
> > freeze and unfreezes every few seconds. Thanks to Eryu Guan
> > ([hidden email]) for writing the test that found this on a system
> > with a somewhat unique default configuration....
>
> That sounds like something that could fit xfstests fairly easily.
>
> Re the patch - you're moving the transaction allocation back into the
> end_io handler.  That's what my original version did, and I'm pretty
> sure you talked me out of it back then.  I can't remember the details
> but the list should have it.

Right, I was concerned about blocking IO completion workers waiting
for log reservations. I'm still concerned about that, but I don't
see any way around it.

>
> > @@ -151,9 +151,11 @@ xfs_setfilesize(
> >   /*
> >   * The transaction was allocated in the I/O submission thread,
> >   * thus we need to mark ourselves as beeing in a transaction
> > - * manually.
> > + * manually. Similarly for freeze protection.
> >   */
> >   current_set_flags_nested(&tp->t_pflags, PF_FSTRANS);
> > + rwsem_acquire_read(&VFS_I(ip)->i_sb->s_writers.lock_map[SB_FREEZE_FS-1],
> > +   0, 1, _THIS_IP_);
>
> The comment above isn't true anymore, and the flags hack should be
> removed.

It's still used by buffered IO in that way.

> I'm also not sure the freeze protection still works if the acquire is
> outside the original broader scope protection.

We've still got a write level freeze reference, so that has to
expire before the transaction level freeze is done. hence I don't
see a problem here.

Indeed, the problem was exposed by freezing and unfreezing every few
seconds and typically tripped within 10-15s of starting. The fix has
survived all night under the same load, so I think the freeze code is
fine.

> I'll defer to Jan on
> this as I don't really understand this magic enough.q
> should be removed respectively replaced with sb_start_intwrite/sb_end_intwrite
> >   if (ioend->io_type == XFS_IO_UNWRITTEN) {
> >   error = xfs_iomap_write_unwritten(ip, ioend->io_offset,
> > +  ioend->io_size);
> > + goto done;
> > + }
> > +
> > + /*
> > + * For direct I/O we do not know if we need to allocate blocks or not so
> > + * we can't preallocate an append transaction as that results in nested
> > + * reservations and log space deadlocks. Hence allocate the transaction
> > + * here.  For buffered I/O we preallocate a transaction when submitting
> > + * the IO.
> > + */
> > + if (ioend->io_isdirect && xfs_ioend_is_append(ioend)) {
>
> xfs_iomap_write_unwritten already updates the inode size, so this should
> be an "else if"

The unwritten branch jumps over this completely if it is taken, so
it makes no difference. I can change it is you want....

>
> > + error = xfs_setfilesize_trans_alloc(ioend);
> > + if (error)
> > + }
> > +
> > + if (ioend->io_append_trans) {
> >   error = xfs_setfilesize(ioend);
> >   } else {
> >   ASSERT(!xfs_ioend_is_append(ioend));
> >   }
>
> Does it really make sense to have different allocation points for
> buffered vs direct I/O? At least it needs a comment explaining why
> it's done differently.

It does, I think, because if we can avoid blocking IO completion for
transaction reservation we should. I'll add a comment.

> While it's probably too much work for a quick fix I'd much rather
> replace the hacks we currently do in the direct I/O code with a
> scheme where the dio code has a hook to allocate the transaction if
> needed at the right point.

The dio code needs an enema. There's plenty wrong with it that needs
optimising (e.g. allocation call on every iovec element) but the
code is so complex, fast-pathy and fragile that it I'm not about to
touch it and complicate what is a relatively simple XFS bug fix...

Cheers,

Dave.
--
Dave Chinner
[hidden email]

_______________________________________________
xfs mailing list
[hidden email]
http://oss.sgi.com/mailman/listinfo/xfs
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] xfs: fix direct IO nested transaction deadlock.

Christoph Hellwig
On Wed, Nov 21, 2012 at 06:53:44AM +1100, Dave Chinner wrote:
> Right, I was concerned about blocking IO completion workers waiting
> for log reservations. I'm still concerned about that, but I don't
> see any way around it.

That's information that should be added to a comment..

> > >   /*
> > >   * The transaction was allocated in the I/O submission thread,
> > >   * thus we need to mark ourselves as beeing in a transaction
> > > - * manually.
> > > + * manually. Similarly for freeze protection.
> > >   */
> > >   current_set_flags_nested(&tp->t_pflags, PF_FSTRANS);
> > > + rwsem_acquire_read(&VFS_I(ip)->i_sb->s_writers.lock_map[SB_FREEZE_FS-1],
> > > +   0, 1, _THIS_IP_);
> >
> > The comment above isn't true anymore, and the flags hack should be
> > removed.
>
> It's still used by buffered IO in that way.

It's conditionaly though, so there should at least be a "may" in the
sentence.

> > >   if (ioend->io_type == XFS_IO_UNWRITTEN) {
> > >   error = xfs_iomap_write_unwritten(ip, ioend->io_offset,
> > > +  ioend->io_size);
> > > + goto done;
> > > + }
> > > +
> > > + /*
> > > + * For direct I/O we do not know if we need to allocate blocks or not so
> > > + * we can't preallocate an append transaction as that results in nested
> > > + * reservations and log space deadlocks. Hence allocate the transaction
> > > + * here.  For buffered I/O we preallocate a transaction when submitting
> > > + * the IO.
> > > + */
> > > + if (ioend->io_isdirect && xfs_ioend_is_append(ioend)) {
> >
> > xfs_iomap_write_unwritten already updates the inode size, so this should
> > be an "else if"
>
> The unwritten branch jumps over this completely if it is taken, so
> it makes no difference. I can change it is you want....

Oh, right - I missed that.  But it seems the else would do the same as
the goto done in your new version, and I generally prefer else if style
control flow for this over gotos.

_______________________________________________
xfs mailing list
[hidden email]
http://oss.sgi.com/mailman/listinfo/xfs
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] xfs: fix direct IO nested transaction deadlock.

Dave Chinner
On Wed, Nov 21, 2012 at 04:59:00AM -0500, Christoph Hellwig wrote:

> On Wed, Nov 21, 2012 at 06:53:44AM +1100, Dave Chinner wrote:
> > Right, I was concerned about blocking IO completion workers waiting
> > for log reservations. I'm still concerned about that, but I don't
> > see any way around it.
>
> That's information that should be added to a comment..
>
> > > >   /*
> > > >   * The transaction was allocated in the I/O submission thread,
> > > >   * thus we need to mark ourselves as beeing in a transaction
> > > > - * manually.
> > > > + * manually. Similarly for freeze protection.
> > > >   */
> > > >   current_set_flags_nested(&tp->t_pflags, PF_FSTRANS);
> > > > + rwsem_acquire_read(&VFS_I(ip)->i_sb->s_writers.lock_map[SB_FREEZE_FS-1],
> > > > +   0, 1, _THIS_IP_);
> > >
> > > The comment above isn't true anymore, and the flags hack should be
> > > removed.
> >
> > It's still used by buffered IO in that way.
>
> It's conditionaly though, so there should at least be a "may" in the
> sentence.

OK.

> > > >   if (ioend->io_type == XFS_IO_UNWRITTEN) {
> > > >   error = xfs_iomap_write_unwritten(ip, ioend->io_offset,
> > > > +  ioend->io_size);
> > > > + goto done;
> > > > + }
> > > > +
> > > > + /*
> > > > + * For direct I/O we do not know if we need to allocate blocks or not so
> > > > + * we can't preallocate an append transaction as that results in nested
> > > > + * reservations and log space deadlocks. Hence allocate the transaction
> > > > + * here.  For buffered I/O we preallocate a transaction when submitting
> > > > + * the IO.
> > > > + */
> > > > + if (ioend->io_isdirect && xfs_ioend_is_append(ioend)) {
> > >
> > > xfs_iomap_write_unwritten already updates the inode size, so this should
> > > be an "else if"
> >
> > The unwritten branch jumps over this completely if it is taken, so
> > it makes no difference. I can change it is you want....
>
> Oh, right - I missed that.  But it seems the else would do the same as
> the goto done in your new version, and I generally prefer else if style
> control flow for this over gotos.

OK, I'll fix that.

Cheers,

Dave.
--
Dave Chinner
[hidden email]

_______________________________________________
xfs mailing list
[hidden email]
http://oss.sgi.com/mailman/listinfo/xfs