* [PATCH 0/2] xfs: regression fixes for 3.8 merge cycle @ 2012-11-20 11:27 Dave Chinner 2012-11-20 11:27 ` [PATCH 1/2] xfs: inode allocation should use unmapped buffers Dave Chinner 2012-11-20 11:27 ` [PATCH 2/2] xfs: fix direct IO nested transaction deadlock Dave Chinner 0 siblings, 2 replies; 9+ messages in thread From: Dave Chinner @ 2012-11-20 11:27 UTC (permalink / raw) To: xfs 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 xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] xfs: inode allocation should use unmapped buffers. 2012-11-20 11:27 [PATCH 0/2] xfs: regression fixes for 3.8 merge cycle Dave Chinner @ 2012-11-20 11:27 ` Dave Chinner 2012-11-20 15:57 ` Christoph Hellwig 2012-11-20 11:27 ` [PATCH 2/2] xfs: fix direct IO nested transaction deadlock Dave Chinner 1 sibling, 1 reply; 9+ messages in thread From: Dave Chinner @ 2012-11-20 11:27 UTC (permalink / raw) To: xfs From: Dave Chinner <dchinner@redhat.com> 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 <dchinner@redhat.com> --- 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 xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] xfs: inode allocation should use unmapped buffers. 2012-11-20 11:27 ` [PATCH 1/2] xfs: inode allocation should use unmapped buffers Dave Chinner @ 2012-11-20 15:57 ` Christoph Hellwig 0 siblings, 0 replies; 9+ messages in thread From: Christoph Hellwig @ 2012-11-20 15:57 UTC (permalink / raw) To: Dave Chinner; +Cc: xfs On Tue, Nov 20, 2012 at 10:27:10PM +1100, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > 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 <dchinner@redhat.com> Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de> _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] xfs: fix direct IO nested transaction deadlock. 2012-11-20 11:27 [PATCH 0/2] xfs: regression fixes for 3.8 merge cycle Dave Chinner 2012-11-20 11:27 ` [PATCH 1/2] xfs: inode allocation should use unmapped buffers Dave Chinner @ 2012-11-20 11:27 ` Dave Chinner 2012-11-20 16:10 ` Christoph Hellwig 1 sibling, 1 reply; 9+ messages in thread From: Dave Chinner @ 2012-11-20 11:27 UTC (permalink / raw) To: xfs From: Dave Chinner <dchinner@redhat.com> 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 (eguan@redhat.com) for writing the test that found this on a system with a somewhat unique default configuration.... cc: <stable@vger.kernel.org> Signed-off-by: Dave Chinner <dchinner@redhat.com> --- 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 xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] xfs: fix direct IO nested transaction deadlock. 2012-11-20 11:27 ` [PATCH 2/2] xfs: fix direct IO nested transaction deadlock Dave Chinner @ 2012-11-20 16:10 ` Christoph Hellwig 2012-11-20 16:37 ` Jan Kara 2012-11-20 19:53 ` Dave Chinner 0 siblings, 2 replies; 9+ messages in thread From: Christoph Hellwig @ 2012-11-20 16:10 UTC (permalink / raw) To: Dave Chinner, Jan Kara; +Cc: xfs 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 > (eguan@redhat.com) 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 xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] xfs: fix direct IO nested transaction deadlock. 2012-11-20 16:10 ` Christoph Hellwig @ 2012-11-20 16:37 ` Jan Kara 2012-11-20 19:53 ` Dave Chinner 1 sibling, 0 replies; 9+ messages in thread From: Jan Kara @ 2012-11-20 16:37 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Jan Kara, xfs 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 > > (eguan@redhat.com) 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 <jack@suse.cz> SUSE Labs, CR _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] xfs: fix direct IO nested transaction deadlock. 2012-11-20 16:10 ` Christoph Hellwig 2012-11-20 16:37 ` Jan Kara @ 2012-11-20 19:53 ` Dave Chinner 2012-11-21 9:59 ` Christoph Hellwig 1 sibling, 1 reply; 9+ messages in thread From: Dave Chinner @ 2012-11-20 19:53 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Jan Kara, xfs 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 > > (eguan@redhat.com) 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 david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] xfs: fix direct IO nested transaction deadlock. 2012-11-20 19:53 ` Dave Chinner @ 2012-11-21 9:59 ` Christoph Hellwig 2012-11-22 0:48 ` Dave Chinner 0 siblings, 1 reply; 9+ messages in thread From: Christoph Hellwig @ 2012-11-21 9:59 UTC (permalink / raw) To: Dave Chinner; +Cc: Christoph Hellwig, Jan Kara, xfs 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 xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] xfs: fix direct IO nested transaction deadlock. 2012-11-21 9:59 ` Christoph Hellwig @ 2012-11-22 0:48 ` Dave Chinner 0 siblings, 0 replies; 9+ messages in thread From: Dave Chinner @ 2012-11-22 0:48 UTC (permalink / raw) To: Christoph Hellwig; +Cc: Jan Kara, xfs 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 david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-11-22 0:46 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-11-20 11:27 [PATCH 0/2] xfs: regression fixes for 3.8 merge cycle Dave Chinner 2012-11-20 11:27 ` [PATCH 1/2] xfs: inode allocation should use unmapped buffers Dave Chinner 2012-11-20 15:57 ` Christoph Hellwig 2012-11-20 11:27 ` [PATCH 2/2] xfs: fix direct IO nested transaction deadlock Dave Chinner 2012-11-20 16:10 ` Christoph Hellwig 2012-11-20 16:37 ` Jan Kara 2012-11-20 19:53 ` Dave Chinner 2012-11-21 9:59 ` Christoph Hellwig 2012-11-22 0:48 ` Dave Chinner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox