* [PATCH 1/6] xfs: silence uninitialised f.file warning.
2012-11-02 0:38 [PATCH 0/6] xfs: fixes for 3.7-rc3 Dave Chinner
@ 2012-11-02 0:38 ` Dave Chinner
2012-11-02 13:04 ` Christoph Hellwig
2012-11-02 21:23 ` Mark Tinguely
2012-11-02 0:38 ` [PATCH 2/6] xfs: growfs: don't read garbage for new secondary superblocks Dave Chinner
` (6 subsequent siblings)
7 siblings, 2 replies; 33+ messages in thread
From: Dave Chinner @ 2012-11-02 0:38 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
Uninitialised variable build warning introduced by 2903ff0 ("switch
simple cases of fget_light to fdget"), gcc is not smart enough to
work out that the variable is not used uninitialised, and the commit
removed the initialisation at declaration that the old variable had.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
fs/xfs/xfs_ioctl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 8305f2a..c1df3c6 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -70,7 +70,7 @@ xfs_find_handle(
int hsize;
xfs_handle_t handle;
struct inode *inode;
- struct fd f;
+ struct fd f = {0};
struct path path;
int error;
struct xfs_inode *ip;
--
1.7.10
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 1/6] xfs: silence uninitialised f.file warning.
2012-11-02 0:38 ` [PATCH 1/6] xfs: silence uninitialised f.file warning Dave Chinner
@ 2012-11-02 13:04 ` Christoph Hellwig
2012-11-02 21:23 ` Mark Tinguely
1 sibling, 0 replies; 33+ messages in thread
From: Christoph Hellwig @ 2012-11-02 13:04 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On Fri, Nov 02, 2012 at 11:38:39AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Uninitialised variable build warning introduced by 2903ff0 ("switch
> simple cases of fget_light to fdget"), gcc is not smart enough to
> work out that the variable is not used uninitialised, and the commit
> removed the initialisation at declaration that the old variable had.
>
> 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] 33+ messages in thread* Re: [PATCH 1/6] xfs: silence uninitialised f.file warning.
2012-11-02 0:38 ` [PATCH 1/6] xfs: silence uninitialised f.file warning Dave Chinner
2012-11-02 13:04 ` Christoph Hellwig
@ 2012-11-02 21:23 ` Mark Tinguely
1 sibling, 0 replies; 33+ messages in thread
From: Mark Tinguely @ 2012-11-02 21:23 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On 11/01/12 19:38, Dave Chinner wrote:
> From: Dave Chinner<dchinner@redhat.com>
>
> Uninitialised variable build warning introduced by 2903ff0 ("switch
> simple cases of fget_light to fdget"), gcc is not smart enough to
> work out that the variable is not used uninitialised, and the commit
> removed the initialisation at declaration that the old variable had.
>
> Signed-off-by: Dave Chinner<dchinner@redhat.com>
> ---
Looks good.
Reviewed-by: Mark Tinguely <tinguely@sgi.com>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 2/6] xfs: growfs: don't read garbage for new secondary superblocks
2012-11-02 0:38 [PATCH 0/6] xfs: fixes for 3.7-rc3 Dave Chinner
2012-11-02 0:38 ` [PATCH 1/6] xfs: silence uninitialised f.file warning Dave Chinner
@ 2012-11-02 0:38 ` Dave Chinner
2012-11-02 0:38 ` [PATCH 3/6] xfs: invalidate allocbt blocks moved to the free list Dave Chinner
` (5 subsequent siblings)
7 siblings, 0 replies; 33+ messages in thread
From: Dave Chinner @ 2012-11-02 0:38 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
When updating new secondary superblocks in a growfs operation, the
sueprblock buffer is read from the newly grown region of the
underlying device. This is not guaranteed to be zero, so violates
the underlying assumption that the unused parts of superblocks are
zero filled. Get a new buffer for these secondary superblocks to
ensure that the unused regions are zero filled correctly.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_fsops.c | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index c25b094..4beaede 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -399,9 +399,26 @@ xfs_growfs_data_private(
/* update secondary superblocks. */
for (agno = 1; agno < nagcount; agno++) {
- error = xfs_trans_read_buf(mp, NULL, mp->m_ddev_targp,
+ error = 0;
+ /*
+ * new secondary superblocks need to be zeroed, not read from
+ * disk as the contents of the new area we are growing into is
+ * completely unknown.
+ */
+ if (agno < oagcount) {
+ error = xfs_trans_read_buf(mp, NULL, mp->m_ddev_targp,
XFS_AGB_TO_DADDR(mp, agno, XFS_SB_BLOCK(mp)),
XFS_FSS_TO_BB(mp, 1), 0, &bp);
+ } else {
+ bp = xfs_trans_get_buf(NULL, mp->m_ddev_targp,
+ XFS_AGB_TO_DADDR(mp, agno, XFS_SB_BLOCK(mp)),
+ XFS_FSS_TO_BB(mp, 1), 0);
+ if (bp)
+ xfs_buf_zero(bp, 0, BBTOB(bp->b_length));
+ else
+ error = ENOMEM;
+ }
+
if (error) {
xfs_warn(mp,
"error %d reading secondary superblock for ag %d",
@@ -423,7 +440,7 @@ xfs_growfs_data_private(
break; /* no point in continuing */
}
}
- return 0;
+ return error;
error0:
xfs_trans_cancel(tp, XFS_TRANS_ABORT);
--
1.7.10
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH 3/6] xfs: invalidate allocbt blocks moved to the free list
2012-11-02 0:38 [PATCH 0/6] xfs: fixes for 3.7-rc3 Dave Chinner
2012-11-02 0:38 ` [PATCH 1/6] xfs: silence uninitialised f.file warning Dave Chinner
2012-11-02 0:38 ` [PATCH 2/6] xfs: growfs: don't read garbage for new secondary superblocks Dave Chinner
@ 2012-11-02 0:38 ` Dave Chinner
2012-10-09 19:11 ` [PATCH] xfs: report projid32bit feature in geometry call Eric Sandeen
2012-11-02 21:23 ` [PATCH 3/6] xfs: invalidate allocbt blocks moved to the free list Mark Tinguely
2012-11-02 0:38 ` [PATCH 4/6] xfs: don't vmap inode cluster buffers during free Dave Chinner
` (4 subsequent siblings)
7 siblings, 2 replies; 33+ messages in thread
From: Dave Chinner @ 2012-11-02 0:38 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
When we free a block from the alloc btree tree, we move it to the
freelist held in the AGFL and mark it busy in the busy extent tree.
This typically happens when we merge btree blocks.
Once the transaction is committed and checkpointed, the block can
remain on the free list for an indefinite amount of time. Now, this
isn't the end of the world at this point - if the free list is
shortened, the buffer is invalidated in the transaction that moves
it back to free space. If the buffer is allocated as metadata from
the free list, then all the modifications getted logged, and we have
no issues, either. And if it gets allocated as userdata direct from
the freelist, it gets invalidated and so will never get written.
However, during the time it sits on the free list, pressure on the
log can cause the AIL to be pushed and the buffer that covers the
block gets pushed for write. IOWs, we end up writing a freed
metadata block to disk. Again, this isn't the end of the world
because we know from the above we are only writing to free space.
The problem, however, is for validation callbacks. If the block was
on old btree root block, then the level of the block is going to be
higher than the current tree root, and so will fail validation.
There may be other inconsistencies in the block as well, and
currently we don't care because the block is in free space. Shutting
down the filesystem because a freed block doesn't pass write
validation, OTOH, is rather unfriendly.
So, make sure we always invalidate buffers as they move from the
free space trees to the free list so that we guarantee they never
get written to disk while on the free list.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_alloc_btree.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/xfs/xfs_alloc_btree.c b/fs/xfs/xfs_alloc_btree.c
index f1647ca..f7876c6 100644
--- a/fs/xfs/xfs_alloc_btree.c
+++ b/fs/xfs/xfs_alloc_btree.c
@@ -121,6 +121,8 @@ xfs_allocbt_free_block(
xfs_extent_busy_insert(cur->bc_tp, be32_to_cpu(agf->agf_seqno), bno, 1,
XFS_EXTENT_BUSY_SKIP_DISCARD);
xfs_trans_agbtree_delta(cur->bc_tp, -1);
+
+ xfs_trans_binval(cur->bc_tp, bp);
return 0;
}
--
1.7.10
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH] xfs: report projid32bit feature in geometry call
@ 2012-10-09 19:11 ` Eric Sandeen
2012-10-09 19:28 ` Carlos Maiolino
` (3 more replies)
0 siblings, 4 replies; 33+ messages in thread
From: Eric Sandeen @ 2012-10-09 19:11 UTC (permalink / raw)
To: xfs-oss
When xfs gained the projid32bit feature, it was never added to
the FSGEOMETRY ioctl feature flags, so it's not queryable without
this patch.
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
note1: is there a reason we had gaps in the flags?
note2: I don't *think* this requires a new version for the ioctl
diff --git a/fs/xfs/xfs_fs.h b/fs/xfs/xfs_fs.h
index c13fed8..0948c04 100644
--- a/fs/xfs/xfs_fs.h
+++ b/fs/xfs/xfs_fs.h
@@ -233,7 +233,8 @@ typedef struct xfs_fsop_resblks {
#define XFS_FSOP_GEOM_FLAGS_LOGV2 0x0100 /* log format version 2 */
#define XFS_FSOP_GEOM_FLAGS_SECTOR 0x0200 /* sector sizes >1BB */
#define XFS_FSOP_GEOM_FLAGS_ATTR2 0x0400 /* inline attributes rework */
-#define XFS_FSOP_GEOM_FLAGS_DIRV2CI 0x1000 /* ASCII only CI names */
+#define XFS_FSOP_GEOM_FLAGS_PROJID32 0x0800 /* 32-bit project IDs */
+#define XFS_FSOP_GEOM_FLAGS_DIRV2CI 0x1000 /* ASCII only CI names */
#define XFS_FSOP_GEOM_FLAGS_LAZYSB 0x4000 /* lazy superblock counters */
diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index c25b094..89ad847 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -97,7 +97,9 @@ xfs_fs_geometry(
(xfs_sb_version_haslazysbcount(&mp->m_sb) ?
XFS_FSOP_GEOM_FLAGS_LAZYSB : 0) |
(xfs_sb_version_hasattr2(&mp->m_sb) ?
- XFS_FSOP_GEOM_FLAGS_ATTR2 : 0);
+ XFS_FSOP_GEOM_FLAGS_ATTR2 : 0) |
+ (xfs_sb_version_hasprojid32bit(&mp->m_sb) ?
+ XFS_FSOP_GEOM_FLAGS_PROJID32 : 0);
geo->logsectsize = xfs_sb_version_hassector(&mp->m_sb) ?
mp->m_sb.sb_logsectsize : BBSIZE;
geo->rtsectsize = mp->m_sb.sb_blocksize;
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH] xfs: report projid32bit feature in geometry call
2012-10-09 19:11 ` [PATCH] xfs: report projid32bit feature in geometry call Eric Sandeen
@ 2012-10-09 19:28 ` Carlos Maiolino
2012-10-09 19:45 ` Dave Chinner
` (2 subsequent siblings)
3 siblings, 0 replies; 33+ messages in thread
From: Carlos Maiolino @ 2012-10-09 19:28 UTC (permalink / raw)
To: xfs
On Tue, Oct 09, 2012 at 02:11:45PM -0500, Eric Sandeen wrote:
> When xfs gained the projid32bit feature, it was never added to
> the FSGEOMETRY ioctl feature flags, so it's not queryable without
> this patch.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>
> note1: is there a reason we had gaps in the flags?
> note2: I don't *think* this requires a new version for the ioctl
>
> diff --git a/fs/xfs/xfs_fs.h b/fs/xfs/xfs_fs.h
> index c13fed8..0948c04 100644
> --- a/fs/xfs/xfs_fs.h
> +++ b/fs/xfs/xfs_fs.h
> @@ -233,7 +233,8 @@ typedef struct xfs_fsop_resblks {
> #define XFS_FSOP_GEOM_FLAGS_LOGV2 0x0100 /* log format version 2 */
> #define XFS_FSOP_GEOM_FLAGS_SECTOR 0x0200 /* sector sizes >1BB */
> #define XFS_FSOP_GEOM_FLAGS_ATTR2 0x0400 /* inline attributes rework */
> -#define XFS_FSOP_GEOM_FLAGS_DIRV2CI 0x1000 /* ASCII only CI names */
> +#define XFS_FSOP_GEOM_FLAGS_PROJID32 0x0800 /* 32-bit project IDs */
> +#define XFS_FSOP_GEOM_FLAGS_DIRV2CI 0x1000 /* ASCII only CI names */
> #define XFS_FSOP_GEOM_FLAGS_LAZYSB 0x4000 /* lazy superblock counters */
>
>
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index c25b094..89ad847 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/fs/xfs/xfs_fsops.c
> @@ -97,7 +97,9 @@ xfs_fs_geometry(
> (xfs_sb_version_haslazysbcount(&mp->m_sb) ?
> XFS_FSOP_GEOM_FLAGS_LAZYSB : 0) |
> (xfs_sb_version_hasattr2(&mp->m_sb) ?
> - XFS_FSOP_GEOM_FLAGS_ATTR2 : 0);
> + XFS_FSOP_GEOM_FLAGS_ATTR2 : 0) |
> + (xfs_sb_version_hasprojid32bit(&mp->m_sb) ?
> + XFS_FSOP_GEOM_FLAGS_PROJID32 : 0);
> geo->logsectsize = xfs_sb_version_hassector(&mp->m_sb) ?
> mp->m_sb.sb_logsectsize : BBSIZE;
> geo->rtsectsize = mp->m_sb.sb_blocksize;
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
Looks good,
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
--
--Carlos
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH] xfs: report projid32bit feature in geometry call
2012-10-09 19:11 ` [PATCH] xfs: report projid32bit feature in geometry call Eric Sandeen
2012-10-09 19:28 ` Carlos Maiolino
@ 2012-10-09 19:45 ` Dave Chinner
2012-10-11 0:02 ` Eric Sandeen
2012-11-08 16:12 ` Ben Myers
3 siblings, 0 replies; 33+ messages in thread
From: Dave Chinner @ 2012-10-09 19:45 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs-oss
On Tue, Oct 09, 2012 at 02:11:45PM -0500, Eric Sandeen wrote:
> When xfs gained the projid32bit feature, it was never added to
> the FSGEOMETRY ioctl feature flags, so it's not queryable without
> this patch.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>
> note1: is there a reason we had gaps in the flags?
No idea. I don't think it matters, though.
> note2: I don't *think* this requires a new version for the ioctl
No, it doesn't - the structure format is unchanged.
Looks good.
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] xfs: report projid32bit feature in geometry call
2012-10-09 19:11 ` [PATCH] xfs: report projid32bit feature in geometry call Eric Sandeen
2012-10-09 19:28 ` Carlos Maiolino
2012-10-09 19:45 ` Dave Chinner
@ 2012-10-11 0:02 ` Eric Sandeen
2012-10-30 19:43 ` Ben Myers
2012-11-08 16:12 ` Ben Myers
3 siblings, 1 reply; 33+ messages in thread
From: Eric Sandeen @ 2012-10-11 0:02 UTC (permalink / raw)
To: Eric Sandeen; +Cc: xfs-oss
On 10/9/12 2:11 PM, Eric Sandeen wrote:
> When xfs gained the projid32bit feature, it was never added to
> the FSGEOMETRY ioctl feature flags, so it's not queryable without
> this patch.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
SGI, can you please add a :
Cc: stable@vger.kernel.org
when this is committed? New stuff will be depending on this and it'd be good to get it munged into stable kernels as well.
> ---
>
> note1: is there a reason we had gaps in the flags?
> note2: I don't *think* this requires a new version for the ioctl
>
> diff --git a/fs/xfs/xfs_fs.h b/fs/xfs/xfs_fs.h
> index c13fed8..0948c04 100644
> --- a/fs/xfs/xfs_fs.h
> +++ b/fs/xfs/xfs_fs.h
> @@ -233,7 +233,8 @@ typedef struct xfs_fsop_resblks {
> #define XFS_FSOP_GEOM_FLAGS_LOGV2 0x0100 /* log format version 2 */
> #define XFS_FSOP_GEOM_FLAGS_SECTOR 0x0200 /* sector sizes >1BB */
> #define XFS_FSOP_GEOM_FLAGS_ATTR2 0x0400 /* inline attributes rework */
> -#define XFS_FSOP_GEOM_FLAGS_DIRV2CI 0x1000 /* ASCII only CI names */
> +#define XFS_FSOP_GEOM_FLAGS_PROJID32 0x0800 /* 32-bit project IDs */
> +#define XFS_FSOP_GEOM_FLAGS_DIRV2CI 0x1000 /* ASCII only CI names */
> #define XFS_FSOP_GEOM_FLAGS_LAZYSB 0x4000 /* lazy superblock counters */
>
>
> diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
> index c25b094..89ad847 100644
> --- a/fs/xfs/xfs_fsops.c
> +++ b/fs/xfs/xfs_fsops.c
> @@ -97,7 +97,9 @@ xfs_fs_geometry(
> (xfs_sb_version_haslazysbcount(&mp->m_sb) ?
> XFS_FSOP_GEOM_FLAGS_LAZYSB : 0) |
> (xfs_sb_version_hasattr2(&mp->m_sb) ?
> - XFS_FSOP_GEOM_FLAGS_ATTR2 : 0);
> + XFS_FSOP_GEOM_FLAGS_ATTR2 : 0) |
> + (xfs_sb_version_hasprojid32bit(&mp->m_sb) ?
> + XFS_FSOP_GEOM_FLAGS_PROJID32 : 0);
> geo->logsectsize = xfs_sb_version_hassector(&mp->m_sb) ?
> mp->m_sb.sb_logsectsize : BBSIZE;
> geo->rtsectsize = mp->m_sb.sb_blocksize;
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH] xfs: report projid32bit feature in geometry call
2012-10-11 0:02 ` Eric Sandeen
@ 2012-10-30 19:43 ` Ben Myers
2012-10-30 19:44 ` Eric Sandeen
0 siblings, 1 reply; 33+ messages in thread
From: Ben Myers @ 2012-10-30 19:43 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Eric Sandeen, xfs-oss
On Wed, Oct 10, 2012 at 07:02:08PM -0500, Eric Sandeen wrote:
> On 10/9/12 2:11 PM, Eric Sandeen wrote:
> > When xfs gained the projid32bit feature, it was never added to
> > the FSGEOMETRY ioctl feature flags, so it's not queryable without
> > this patch.
> >
> > Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>
> SGI, can you please add a :
>
> Cc: stable@vger.kernel.org
>
> when this is committed? New stuff will be depending on this and it'd be good to get it munged into stable kernels as well.
Yep. Will do so.
-Ben
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] xfs: report projid32bit feature in geometry call
2012-10-30 19:43 ` Ben Myers
@ 2012-10-30 19:44 ` Eric Sandeen
0 siblings, 0 replies; 33+ messages in thread
From: Eric Sandeen @ 2012-10-30 19:44 UTC (permalink / raw)
To: Ben Myers; +Cc: Eric Sandeen, xfs-oss
On 10/30/12 2:43 PM, Ben Myers wrote:
> On Wed, Oct 10, 2012 at 07:02:08PM -0500, Eric Sandeen wrote:
>> On 10/9/12 2:11 PM, Eric Sandeen wrote:
>>> When xfs gained the projid32bit feature, it was never added to
>>> the FSGEOMETRY ioctl feature flags, so it's not queryable without
>>> this patch.
>>>
>>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>>
>> SGI, can you please add a :
>>
>> Cc: stable@vger.kernel.org
>>
>> when this is committed? New stuff will be depending on this and it'd be good to get it munged into stable kernels as well.
>
> Yep. Will do so.
actually in the end I didn't depend on it in xfsdump, so it's up to you about -stable.
Thanks,
-Eric
> -Ben
>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH] xfs: report projid32bit feature in geometry call
2012-10-09 19:11 ` [PATCH] xfs: report projid32bit feature in geometry call Eric Sandeen
` (2 preceding siblings ...)
2012-10-11 0:02 ` Eric Sandeen
@ 2012-11-08 16:12 ` Ben Myers
3 siblings, 0 replies; 33+ messages in thread
From: Ben Myers @ 2012-11-08 16:12 UTC (permalink / raw)
To: Eric Sandeen, Dave Chinner; +Cc: xfs-oss
On Tue, Oct 09, 2012 at 02:11:45PM -0500, Eric Sandeen wrote:
> When xfs gained the projid32bit feature, it was never added to
> the FSGEOMETRY ioctl feature flags, so it's not queryable without
> this patch.
>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
committed to git://oss.sgi.com/xfs/xfs.git, master and for-next branches.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 3/6] xfs: invalidate allocbt blocks moved to the free list
2012-11-02 0:38 ` [PATCH 3/6] xfs: invalidate allocbt blocks moved to the free list Dave Chinner
2012-10-09 19:11 ` [PATCH] xfs: report projid32bit feature in geometry call Eric Sandeen
@ 2012-11-02 21:23 ` Mark Tinguely
1 sibling, 0 replies; 33+ messages in thread
From: Mark Tinguely @ 2012-11-02 21:23 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On 11/01/12 19:38, Dave Chinner wrote:
> From: Dave Chinner<dchinner@redhat.com>
>
> When we free a block from the alloc btree tree, we move it to the
> freelist held in the AGFL and mark it busy in the busy extent tree.
> This typically happens when we merge btree blocks.
>
> Once the transaction is committed and checkpointed, the block can
> remain on the free list for an indefinite amount of time. Now, this
> isn't the end of the world at this point - if the free list is
> shortened, the buffer is invalidated in the transaction that moves
> it back to free space. If the buffer is allocated as metadata from
> the free list, then all the modifications getted logged, and we have
> no issues, either. And if it gets allocated as userdata direct from
> the freelist, it gets invalidated and so will never get written.
>
> However, during the time it sits on the free list, pressure on the
> log can cause the AIL to be pushed and the buffer that covers the
> block gets pushed for write. IOWs, we end up writing a freed
> metadata block to disk. Again, this isn't the end of the world
> because we know from the above we are only writing to free space.
>
> The problem, however, is for validation callbacks. If the block was
> on old btree root block, then the level of the block is going to be
> higher than the current tree root, and so will fail validation.
> There may be other inconsistencies in the block as well, and
> currently we don't care because the block is in free space. Shutting
> down the filesystem because a freed block doesn't pass write
> validation, OTOH, is rather unfriendly.
>
> So, make sure we always invalidate buffers as they move from the
> free space trees to the free list so that we guarantee they never
> get written to disk while on the free list.
>
> Signed-off-by: Dave Chinner<dchinner@redhat.com>
> Reviewed-by: Christoph Hellwig<hch@lst.de>
Looks good.
Reviewed-by: Mark Tinguely <tinguely@sgi.com>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 4/6] xfs: don't vmap inode cluster buffers during free
2012-11-02 0:38 [PATCH 0/6] xfs: fixes for 3.7-rc3 Dave Chinner
` (2 preceding siblings ...)
2012-11-02 0:38 ` [PATCH 3/6] xfs: invalidate allocbt blocks moved to the free list Dave Chinner
@ 2012-11-02 0:38 ` Dave Chinner
2012-11-02 13:05 ` Christoph Hellwig
2012-11-02 21:24 ` Mark Tinguely
2012-11-02 0:38 ` [PATCH 5/6] xfs: fix buffer shudown reference count mismatch Dave Chinner
` (3 subsequent siblings)
7 siblings, 2 replies; 33+ messages in thread
From: Dave Chinner @ 2012-11-02 0:38 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_inode.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index bba8f37..6a0a710 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1760,7 +1760,8 @@ xfs_ifree_cluster(
* to mark all the active inodes on the buffer stale.
*/
bp = xfs_trans_get_buf(tp, mp->m_ddev_targp, blkno,
- mp->m_bsize * blks_per_cluster, 0);
+ mp->m_bsize * blks_per_cluster,
+ XBF_UNMAPPED);
if (!bp)
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] 33+ messages in thread* [PATCH 5/6] xfs: fix buffer shudown reference count mismatch
2012-11-02 0:38 [PATCH 0/6] xfs: fixes for 3.7-rc3 Dave Chinner
` (3 preceding siblings ...)
2012-11-02 0:38 ` [PATCH 4/6] xfs: don't vmap inode cluster buffers during free Dave Chinner
@ 2012-11-02 0:38 ` Dave Chinner
2012-11-02 2:43 ` Dave Chinner
2012-11-02 0:38 ` [PATCH 6/6] xfs: fix reading of wrapped log data Dave Chinner
` (2 subsequent siblings)
7 siblings, 1 reply; 33+ messages in thread
From: Dave Chinner @ 2012-11-02 0:38 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
When we shut down the filesystem, we have to unpin and free all the
buffers currently active in the CIL. To do this we unpin and remove
them in one operation as a result of a failed iclogbuf write. For
buffers, we do this removal via a simultated IO completion of after
marking the buffer stale.
At the time we do this, we have two references to the buffer - the
active LRU reference and the buf log item. The LRU reference is
removed by marking the buffer stale, and the active CIL reference is
by the xfs_buf_iodone() callback that is run by
xfs_buf_do_callbacks() during ioend processing (via the bp->b_iodone
callback).
However, ioend processing requires one more reference - that of the
IO that it is completing. We don't have this reference, so we free
the buffer prematurely and use it after it is freed. This leads to
assert failures in xfs_buf_rele() on debug kernels because the
b_hold count is zero.
Fix this by making sure we take the necessary IO reference before
starting IO completion processing on the stale buffer.
Cc: <stable@vger.kernel.org>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
fs/xfs/xfs_buf_item.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index a8d0ed9..b72fe88 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -526,7 +526,23 @@ xfs_buf_item_unpin(
}
xfs_buf_relse(bp);
} else if (freed && remove) {
+ /*
+ * There are currently two references to the buffer - the active
+ * LRU reference and the buf log item. What we are about to do
+ * here - simulate a failed IO completion - requires 3
+ * references.
+ *
+ * The LRU reference is removed by the xfs_buf_stale() call. The
+ * buf item reference is removed by the xfs_buf_iodone()
+ * callback that is run by xfs_buf_do_callbacks() during ioend
+ * processing (via the bp->b_iodone callback), and then finally
+ * the ioend processing drops the IO reference.
+ *
+ * Hence we need to take an additional reference here so that IO
+ * completion processing doesn't free the buffer prematurely.
+ */
xfs_buf_lock(bp);
+ xfs_buf_hold(bp);
xfs_buf_ioerror(bp, EIO);
XFS_BUF_UNDONE(bp);
xfs_buf_stale(bp);
--
1.7.10
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 5/6] xfs: fix buffer shudown reference count mismatch
2012-11-02 0:38 ` [PATCH 5/6] xfs: fix buffer shudown reference count mismatch Dave Chinner
@ 2012-11-02 2:43 ` Dave Chinner
2012-11-02 3:23 ` [PATCH 5/6 V2] " Dave Chinner
2012-11-02 13:13 ` [PATCH 5/6] " Christoph Hellwig
0 siblings, 2 replies; 33+ messages in thread
From: Dave Chinner @ 2012-11-02 2:43 UTC (permalink / raw)
To: xfs
On Fri, Nov 02, 2012 at 11:38:43AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> When we shut down the filesystem, we have to unpin and free all the
> buffers currently active in the CIL. To do this we unpin and remove
> them in one operation as a result of a failed iclogbuf write. For
> buffers, we do this removal via a simultated IO completion of after
> marking the buffer stale.
>
> At the time we do this, we have two references to the buffer - the
> active LRU reference and the buf log item. The LRU reference is
> removed by marking the buffer stale, and the active CIL reference is
> by the xfs_buf_iodone() callback that is run by
> xfs_buf_do_callbacks() during ioend processing (via the bp->b_iodone
> callback).
>
> However, ioend processing requires one more reference - that of the
> IO that it is completing. We don't have this reference, so we free
> the buffer prematurely and use it after it is freed. This leads to
> assert failures in xfs_buf_rele() on debug kernels because the
> b_hold count is zero.
>
> Fix this by making sure we take the necessary IO reference before
> starting IO completion processing on the stale buffer.
>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
HOld off on this one for a little while - I think I've found the
source of the subsequent failures that Mark reported.
The reason why this did't fail on every buffer is that not all
buffers drop a reference in xfs_buf_iodone_work(). Only buffers that
are marked with XBF_ASYNC will do this, and they seem to be rare.
Hence the rare failure.
The fix that I've done here means all buffers going through this
path will take an extra reference, but that reference is only
dropped on async buffers. Because all the buffers are markd stale,
they are removed from the LRU, and so xfs_buftarg_wait() during
unmount does not find them and hence the remaining reference is
never removed. Hence the perag reference still remains, and we
assert fail there.
Solution seems simple - set the XBF_ASYNC flag on all buffers so
that the last reference is taken away correctly. Testing that now.
(FWIW, I found this using perag get/put tracing :)
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] 33+ messages in thread
* [PATCH 5/6 V2] xfs: fix buffer shudown reference count mismatch
2012-11-02 2:43 ` Dave Chinner
@ 2012-11-02 3:23 ` Dave Chinner
2012-11-02 13:17 ` Mark Tinguely
2012-11-02 13:13 ` [PATCH 5/6] " Christoph Hellwig
1 sibling, 1 reply; 33+ messages in thread
From: Dave Chinner @ 2012-11-02 3:23 UTC (permalink / raw)
To: xfs
xfs: fix buffer shudown reference count mismatch
From: Dave Chinner <dchinner@redhat.com>
When we shut down the filesystem, we have to unpin and free all the
buffers currently active in the CIL. To do this we unpin and remove
them in one operation as a result of a failed iclogbuf write. For
buffers, we do this removal via a simultated IO completion of after
marking the buffer stale.
At the time we do this, we have two references to the buffer - the
active LRU reference and the buf log item. The LRU reference is
removed by marking the buffer stale, and the active CIL reference is
by the xfs_buf_iodone() callback that is run by
xfs_buf_do_callbacks() during ioend processing (via the bp->b_iodone
callback).
However, ioend processing requires one more reference - that of the
IO that it is completing. We don't have this reference, so we free
the buffer prematurely and use it after it is freed. For buffers
marked with XBF_ASYNC, this leads to assert failures in
xfs_buf_rele() on debug kernels because the b_hold count is zero.
Fix this by making sure we take the necessary IO reference before
starting IO completion processing on the stale buffer, and set the
XBF_ASYNC flag to ensure that IO completion processing removes all
the active references from the buffer to ensure it is fully torn
down.
Cc: <stable@vger.kernel.org>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
V2 - add XBF_ASYNC to buffers so last reference is always removed.
Fixes shutdown assert failures due to references from buffers
remaining the perag structures at unmount.
fs/xfs/xfs_buf_item.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index a8d0ed9..becf4a9 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -526,7 +526,25 @@ xfs_buf_item_unpin(
}
xfs_buf_relse(bp);
} else if (freed && remove) {
+ /*
+ * There are currently two references to the buffer - the active
+ * LRU reference and the buf log item. What we are about to do
+ * here - simulate a failed IO completion - requires 3
+ * references.
+ *
+ * The LRU reference is removed by the xfs_buf_stale() call. The
+ * buf item reference is removed by the xfs_buf_iodone()
+ * callback that is run by xfs_buf_do_callbacks() during ioend
+ * processing (via the bp->b_iodone callback), and then finally
+ * the ioend processing will drop the IO reference if the buffer
+ * is marked XBF_ASYNC.
+ *
+ * Hence we need to take an additional reference here so that IO
+ * completion processing doesn't free the buffer prematurely.
+ */
xfs_buf_lock(bp);
+ xfs_buf_hold(bp);
+ bp->b_flags |= XBF_ASYNC;
xfs_buf_ioerror(bp, EIO);
XFS_BUF_UNDONE(bp);
xfs_buf_stale(bp);
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 5/6 V2] xfs: fix buffer shudown reference count mismatch
2012-11-02 3:23 ` [PATCH 5/6 V2] " Dave Chinner
@ 2012-11-02 13:17 ` Mark Tinguely
0 siblings, 0 replies; 33+ messages in thread
From: Mark Tinguely @ 2012-11-02 13:17 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On 11/01/12 22:23, Dave Chinner wrote:
> xfs: fix buffer shudown reference count mismatch
>
> From: Dave Chinner<dchinner@redhat.com>
>
> When we shut down the filesystem, we have to unpin and free all the
> buffers currently active in the CIL. To do this we unpin and remove
> them in one operation as a result of a failed iclogbuf write. For
> buffers, we do this removal via a simultated IO completion of after
> marking the buffer stale.
>
> At the time we do this, we have two references to the buffer - the
> active LRU reference and the buf log item. The LRU reference is
> removed by marking the buffer stale, and the active CIL reference is
> by the xfs_buf_iodone() callback that is run by
> xfs_buf_do_callbacks() during ioend processing (via the bp->b_iodone
> callback).
>
> However, ioend processing requires one more reference - that of the
> IO that it is completing. We don't have this reference, so we free
> the buffer prematurely and use it after it is freed. For buffers
> marked with XBF_ASYNC, this leads to assert failures in
> xfs_buf_rele() on debug kernels because the b_hold count is zero.
>
> Fix this by making sure we take the necessary IO reference before
> starting IO completion processing on the stale buffer, and set the
> XBF_ASYNC flag to ensure that IO completion processing removes all
> the active references from the buffer to ensure it is fully torn
> down.
>
> Cc:<stable@vger.kernel.org>
> Signed-off-by: Dave Chinner<dchinner@redhat.com>
> ---
> V2 - add XBF_ASYNC to buffers so last reference is always removed.
> Fixes shutdown assert failures due to references from buffers
> remaining the perag structures at unmount.
Yes and thank-you. This eliminates both the b_hold and pag_ref
asserts in xfstest 179.
Reviewed-by: Mark Tinguely <tinguely@sgi.com>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 5/6] xfs: fix buffer shudown reference count mismatch
2012-11-02 2:43 ` Dave Chinner
2012-11-02 3:23 ` [PATCH 5/6 V2] " Dave Chinner
@ 2012-11-02 13:13 ` Christoph Hellwig
2012-11-02 17:10 ` Mark Tinguely
2012-11-02 23:47 ` Dave Chinner
1 sibling, 2 replies; 33+ messages in thread
From: Christoph Hellwig @ 2012-11-02 13:13 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
> The fix that I've done here means all buffers going through this
> path will take an extra reference, but that reference is only
> dropped on async buffers. Because all the buffers are markd stale,
> they are removed from the LRU, and so xfs_buftarg_wait() during
> unmount does not find them and hence the remaining reference is
> never removed. Hence the perag reference still remains, and we
> assert fail there.
>
> Solution seems simple - set the XBF_ASYNC flag on all buffers so
> that the last reference is taken away correctly. Testing that now.
I don't like this. ioend processing is very different for synchrous
writes, with the most important difference being that synchronous
writes need to wake the submitter at I/O completion. From all I can
see your v2 patch breaks that beahviour. For 3.7-rc I'd suggest
taking the additional reference conditionally.
For 3.8 I'm going to look into simply acquiring an additional reference
for synchronous writes during I/O submission to kill these special cases
all over the buffer code.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 5/6] xfs: fix buffer shudown reference count mismatch
2012-11-02 13:13 ` [PATCH 5/6] " Christoph Hellwig
@ 2012-11-02 17:10 ` Mark Tinguely
2012-11-02 23:47 ` Dave Chinner
1 sibling, 0 replies; 33+ messages in thread
From: Mark Tinguely @ 2012-11-02 17:10 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On 11/02/12 08:13, Christoph Hellwig wrote:
> I don't like this. ioend processing is very different for synchrous
> writes, with the most important difference being that synchronous
> writes need to wake the submitter at I/O completion. From all I can
> see your v2 patch breaks that beahviour. For 3.7-rc I'd suggest
> taking the additional reference conditionally.
>
> For 3.8 I'm going to look into simply acquiring an additional reference
> for synchronous writes during I/O submission to kill these special cases
> all over the buffer code.
Testing shows that conditionally taking the additional reference does
work at avoiding the b_bhold is zero assert without causing pag is not
empty assert.
Thanks,
--Mark Tinguely.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 5/6] xfs: fix buffer shudown reference count mismatch
2012-11-02 13:13 ` [PATCH 5/6] " Christoph Hellwig
2012-11-02 17:10 ` Mark Tinguely
@ 2012-11-02 23:47 ` Dave Chinner
2012-11-06 12:59 ` Christoph Hellwig
1 sibling, 1 reply; 33+ messages in thread
From: Dave Chinner @ 2012-11-02 23:47 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Fri, Nov 02, 2012 at 09:13:26AM -0400, Christoph Hellwig wrote:
> > The fix that I've done here means all buffers going through this
> > path will take an extra reference, but that reference is only
> > dropped on async buffers. Because all the buffers are markd stale,
> > they are removed from the LRU, and so xfs_buftarg_wait() during
> > unmount does not find them and hence the remaining reference is
> > never removed. Hence the perag reference still remains, and we
> > assert fail there.
> >
> > Solution seems simple - set the XBF_ASYNC flag on all buffers so
> > that the last reference is taken away correctly. Testing that now.
>
> I don't like this. ioend processing is very different for synchrous
> writes, with the most important difference being that synchronous
> writes need to wake the submitter at I/O completion.
I think that's irrelevant here - there will *never* be an IO waiter
at this point in time. This processing is in log buffer IO
completion context, so the buffers are still pinned in memory. Hence
anyone trying to do IO on it will be waiting in xfs_buf_wait_unpin()
and never get to xfs_buf_iowait(). And because xfs_buf_wait_unpin()
is called with the buffer lock held, we'll never do the failure
handling in xfs_buf_item_unpin until the buffer IO is completed and
it is unlocked.
FWIW, this also indicates problem in IO submission -
xfs_buf_wait_unpin() occurs after the shutdown checks, so we could
wake a pinned waiter that then issues IO after the shutdown has
started because there are no shutdown checks after
xfs_buf_wait_unpin() is called....
> From all I can
> see your v2 patch breaks that beahviour. For 3.7-rc I'd suggest
> taking the additional reference conditionally.
It's not a conditional reference - every buffer needs it. Right now
we have an unconditional use after free because bp->b_iodone() drops
the last reference on the buffer, then calls xfs_buf_iodone() again.
The only thing the XBF_ASYNC flag cwcurrently determines is is
whether we get an assert failure by calling xfs_buf_rele() on a
freed buffer or not.
What we are doing here is ioend processing without actually having
issued an IO. i.e. we are using the ioend processing for error
handling, not for IO completion. Hence we have to set the buffer up
as though it has been under IO so that ioend processing does exactly
what we want. And what we want is the buffer to be unconditionally
freed at the end of ioend processing.
What we end up with when we hit the "remove && freed" case is
a buffer that is not pinned, is not under IO and will have
no new references because we are in a shutdown situation. We need to
mark the buffer stale to ensure it is not written, and free the
buffer when the last reference goes away. That reference is the
reference the transaction subsystem owns and it guarantees that
there is no IO completion waiters on the buffer.
IOWs, we have two options after taking an extra reference:
1. clear the XBF_ASYNC flag unconditionally and call
xfs_buf_relse() ourselves to unlock and free the buffer
2. set the XBF_ASYNC flag unconditionally and let
xfs_buf_ioend() call xfs_buf_relse() itself.
I tossed a coin, and it came down on #2. Either way will work, but
we need that extra reference on all buffers passing through this
code...
> For 3.8 I'm going to look into simply acquiring an additional reference
> for synchronous writes during I/O submission to kill these special cases
> all over the buffer code.
Sure, but that's a different issue, not directly realted to fixing
the bug at hand. It would also mean that if we choose #1, we'll need
to take 2 references for the IO for this error handling path. That
seems like a much stranger special case than just using async buffer
IO completion to do the work we need...
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] 33+ messages in thread
* Re: [PATCH 5/6] xfs: fix buffer shudown reference count mismatch
2012-11-02 23:47 ` Dave Chinner
@ 2012-11-06 12:59 ` Christoph Hellwig
2012-11-06 19:59 ` Dave Chinner
0 siblings, 1 reply; 33+ messages in thread
From: Christoph Hellwig @ 2012-11-06 12:59 UTC (permalink / raw)
To: Dave Chinner; +Cc: Christoph Hellwig, xfs
On Sat, Nov 03, 2012 at 10:47:41AM +1100, Dave Chinner wrote:
> I think that's irrelevant here - there will *never* be an IO waiter
> at this point in time. This processing is in log buffer IO
> completion context, so the buffers are still pinned in memory. Hence
> anyone trying to do IO on it will be waiting in xfs_buf_wait_unpin()
> and never get to xfs_buf_iowait(). And because xfs_buf_wait_unpin()
> is called with the buffer lock held, we'll never do the failure
> handling in xfs_buf_item_unpin until the buffer IO is completed and
> it is unlocked.
How do we manage to submit it synchronously then? The inode and dquot
reclaim xfs_bwrite calls already wait for an unpin first, so I don't
think these are the problem. The only other call on a live fs seems
to xfs_qm_shake -> xfs_buf_delwri_submit, but that one does wait
for the complete() call on b_iowait. I suspect we are hitting that
and due to it skipping the wait if b_ioerror is set and waiting on
multiple buffers that complete together might hide the issue.
__xfs_buf_delwri_submit for the wait == true case also seems to be
the only place that actually skips the ispinned check.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 5/6] xfs: fix buffer shudown reference count mismatch
2012-11-06 12:59 ` Christoph Hellwig
@ 2012-11-06 19:59 ` Dave Chinner
0 siblings, 0 replies; 33+ messages in thread
From: Dave Chinner @ 2012-11-06 19:59 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Tue, Nov 06, 2012 at 07:59:49AM -0500, Christoph Hellwig wrote:
> On Sat, Nov 03, 2012 at 10:47:41AM +1100, Dave Chinner wrote:
> > I think that's irrelevant here - there will *never* be an IO waiter
> > at this point in time. This processing is in log buffer IO
> > completion context, so the buffers are still pinned in memory. Hence
> > anyone trying to do IO on it will be waiting in xfs_buf_wait_unpin()
> > and never get to xfs_buf_iowait(). And because xfs_buf_wait_unpin()
> > is called with the buffer lock held, we'll never do the failure
> > handling in xfs_buf_item_unpin until the buffer IO is completed and
> > it is unlocked.
>
> How do we manage to submit it synchronously then?
I don't follow what problem you are talking about here.
Fundamentally, races with IO are resolved like this regardless of
whether the racing Io is sync or async
xfs_buf_lock
make modifications
.....
xfs_buf_lock
.....
xfs_trans_commit
....
IOP_PIN()
IOP_UNLOCK()
xfs_buf_iorequest
xfs_buf_wait_unpin()
.....
<shutdown, no new buffers can get to xfs_buf_iorequest>
IOP_UNPIN(remove)
xfs_buf_item_unpin(remove)
wake_up_all(pin waiters)
xfs_buf_lock()
.....
submit IO
......
xfs_buf_ioend()
wakeup(b_iowait)
.....
xfs_buf_relse()
xfs_buf_hold
xfs_buf_stale
ASYNC
xfs_buf_ioend()
bp->b_iodone()
xfs_buf_rele
xfs_buf_ioend()
xfs_buf_rele
xfs_buf_free
What this also points out is that we shoul dbe checking for shutdown
after xfs_buf_wait_unpin(), too, because otherwise we are submitting
IO after the shutdown is initiated....
> The inode and dquot
> reclaim xfs_bwrite calls already wait for an unpin first, so I don't
> think these are the problem. The only other call on a live fs seems
> to xfs_qm_shake -> xfs_buf_delwri_submit, but that one does wait
> for the complete() call on b_iowait. I suspect we are hitting that
> and due to it skipping the wait if b_ioerror is set and waiting on
> multiple buffers that complete together might hide the issue.
We are in a shutdown situation. xfs_buf_delwri_submit() goes via
xfs_bdstrat_cb() which will stop any new IOs from being submitted
via this path. If it is blocked on the above case, then it is
also resolved by the above case...
> __xfs_buf_delwri_submit for the wait == true case also seems to be
> the only place that actually skips the ispinned check.
Sure, but that we're in a shutdown situation, so it doesn't matter -
the buffer will never get to xfs_buf_wait_unpin() because of the
shutdown check in xfs_bdstrat_cb().
I still don't see the problem you are trying to explain to me. Maybe
I'm just being dense....
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] 33+ messages in thread
* [PATCH 6/6] xfs: fix reading of wrapped log data
2012-11-02 0:38 [PATCH 0/6] xfs: fixes for 3.7-rc3 Dave Chinner
` (4 preceding siblings ...)
2012-11-02 0:38 ` [PATCH 5/6] xfs: fix buffer shudown reference count mismatch Dave Chinner
@ 2012-11-02 0:38 ` Dave Chinner
2012-11-02 13:07 ` Christoph Hellwig
2012-11-02 21:24 ` Mark Tinguely
2012-11-07 20:56 ` [PATCH 0/6] xfs: fixes for 3.7-rc3 Dave Chinner
2012-11-08 16:15 ` Ben Myers
7 siblings, 2 replies; 33+ messages in thread
From: Dave Chinner @ 2012-11-02 0:38 UTC (permalink / raw)
To: xfs
From: Dave Chinner <dchinner@redhat.com>
Commit 4439647 ("xfs: reset buffer pointers before freeing them") in
3.0-rc1 introduced a regression when recovering log buffers that
wrapped around the end of log. The second part of the log buffer at
the start of the physical log was being read into the header buffer
rather than the data buffer, and hence recovery was seeing garbage
in the data buffer when it got to the region of the log buffer that
was incorrectly read.
Cc: <stable@vger.kernel.org> # 3.0.x, 3.2.x, 3.4.x 3.6.x
Reported-by: Torsten Kaiser <just.for.lkml@googlemail.com>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
fs/xfs/xfs_log_recover.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 651c988..3e06333 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3542,7 +3542,7 @@ xlog_do_recovery_pass(
* - order is important.
*/
error = xlog_bread_offset(log, 0,
- bblks - split_bblks, hbp,
+ bblks - split_bblks, dbp,
offset + BBTOB(split_bblks));
if (error)
goto bread_err2;
--
1.7.10
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH 6/6] xfs: fix reading of wrapped log data
2012-11-02 0:38 ` [PATCH 6/6] xfs: fix reading of wrapped log data Dave Chinner
@ 2012-11-02 13:07 ` Christoph Hellwig
2012-11-02 23:51 ` Dave Chinner
2012-11-02 21:24 ` Mark Tinguely
1 sibling, 1 reply; 33+ messages in thread
From: Christoph Hellwig @ 2012-11-02 13:07 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On Fri, Nov 02, 2012 at 11:38:44AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Commit 4439647 ("xfs: reset buffer pointers before freeing them") in
> 3.0-rc1 introduced a regression when recovering log buffers that
> wrapped around the end of log. The second part of the log buffer at
> the start of the physical log was being read into the header buffer
> rather than the data buffer, and hence recovery was seeing garbage
> in the data buffer when it got to the region of the log buffer that
> was incorrectly read.
I think you mentioned earlier you found a reproducer for this, any
chance it could be added to xfstests?
> error = xlog_bread_offset(log, 0,
> - bblks - split_bblks, hbp,
> + bblks - split_bblks, dbp,
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] 33+ messages in thread* Re: [PATCH 6/6] xfs: fix reading of wrapped log data
2012-11-02 13:07 ` Christoph Hellwig
@ 2012-11-02 23:51 ` Dave Chinner
0 siblings, 0 replies; 33+ messages in thread
From: Dave Chinner @ 2012-11-02 23:51 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Fri, Nov 02, 2012 at 09:07:00AM -0400, Christoph Hellwig wrote:
> On Fri, Nov 02, 2012 at 11:38:44AM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > Commit 4439647 ("xfs: reset buffer pointers before freeing them") in
> > 3.0-rc1 introduced a regression when recovering log buffers that
> > wrapped around the end of log. The second part of the log buffer at
> > the start of the physical log was being read into the header buffer
> > rather than the data buffer, and hence recovery was seeing garbage
> > in the data buffer when it got to the region of the log buffer that
> > was incorrectly read.
>
> I think you mentioned earlier you found a reproducer for this, any
> chance it could be added to xfstests?
My reproducer is specific to the hardware I was testing on and
various filesystem parameters (e.g. log size, RAM, writeback rates
to storage, etc), and it was dependent on compilebench to generate
the load. I've been trying to isolate it down to a reproducable case
simliar to test 016, but I haven't quite got it there yet.
And, yes, I very much want a test that covers recovery of wrapped
log buffers. I was very surprised when I realised that it wasnt'
specifically tested by xfstests....
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] 33+ messages in thread
* Re: [PATCH 6/6] xfs: fix reading of wrapped log data
2012-11-02 0:38 ` [PATCH 6/6] xfs: fix reading of wrapped log data Dave Chinner
2012-11-02 13:07 ` Christoph Hellwig
@ 2012-11-02 21:24 ` Mark Tinguely
1 sibling, 0 replies; 33+ messages in thread
From: Mark Tinguely @ 2012-11-02 21:24 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On 11/01/12 19:38, Dave Chinner wrote:
> From: Dave Chinner<dchinner@redhat.com>
>
> Commit 4439647 ("xfs: reset buffer pointers before freeing them") in
> 3.0-rc1 introduced a regression when recovering log buffers that
> wrapped around the end of log. The second part of the log buffer at
> the start of the physical log was being read into the header buffer
> rather than the data buffer, and hence recovery was seeing garbage
> in the data buffer when it got to the region of the log buffer that
> was incorrectly read.
>
> Cc:<stable@vger.kernel.org> # 3.0.x, 3.2.x, 3.4.x 3.6.x
> Reported-by: Torsten Kaiser<just.for.lkml@googlemail.com>
> Signed-off-by: Dave Chinner<dchinner@redhat.com>
> ---
Looks good.
Reviewed-by: Mark Tinguely <tinguely@sgi.com>
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 0/6] xfs: fixes for 3.7-rc3
2012-11-02 0:38 [PATCH 0/6] xfs: fixes for 3.7-rc3 Dave Chinner
` (5 preceding siblings ...)
2012-11-02 0:38 ` [PATCH 6/6] xfs: fix reading of wrapped log data Dave Chinner
@ 2012-11-07 20:56 ` Dave Chinner
2012-11-08 16:34 ` Ben Myers
2012-11-08 16:15 ` Ben Myers
7 siblings, 1 reply; 33+ messages in thread
From: Dave Chinner @ 2012-11-07 20:56 UTC (permalink / raw)
To: xfs
On Fri, Nov 02, 2012 at 11:38:38AM +1100, Dave Chinner wrote:
> Hi Ben,
>
> Here are my current outstanding fixes that are candidates for 3.7.
> Patches 2 and 3 are the initial fixes from the verifier series (real
> bugs) and the other are regressions that I've recently fixed. Some
> need to be sent back to stable kernels as well and are tagged as
> such.
Hi Ben,
What's the status with these? It's been almost a week since I posted
them, and apart from patch 5, they are all good to go to Linus. IMO,
patch 5 is also good to go as well, but I don't mind if that is not
sent immediately as that one is not as critical as the others.
I'm particularly concerned because of the bug that patch 6 fixes -
it's a real corruption bug that users are hitting we need to get it
upstream and back into the stable kernels ASAP. But this patch
hasn't even been committed to the dev tree yet...
Can you please get these upstream ASAP? If you don't have the time
to do it today, let me know so I can send the patches straight to
Linus and get them to the upstream and stable trees that way....
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] 33+ messages in thread* Re: [PATCH 0/6] xfs: fixes for 3.7-rc3
2012-11-07 20:56 ` [PATCH 0/6] xfs: fixes for 3.7-rc3 Dave Chinner
@ 2012-11-08 16:34 ` Ben Myers
0 siblings, 0 replies; 33+ messages in thread
From: Ben Myers @ 2012-11-08 16:34 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
Hi Dave,
On Thu, Nov 08, 2012 at 07:56:21AM +1100, Dave Chinner wrote:
> On Fri, Nov 02, 2012 at 11:38:38AM +1100, Dave Chinner wrote:
> > Here are my current outstanding fixes that are candidates for 3.7.
> > Patches 2 and 3 are the initial fixes from the verifier series (real
> > bugs) and the other are regressions that I've recently fixed. Some
> > need to be sent back to stable kernels as well and are tagged as
> > such.
>
> Hi Ben,
>
> What's the status with these? It's been almost a week since I posted
> them, and apart from patch 5, they are all good to go to Linus. IMO,
> patch 5 is also good to go as well, but I don't mind if that is not
> sent immediately as that one is not as critical as the others.
>
> I'm particularly concerned because of the bug that patch 6 fixes -
> it's a real corruption bug that users are hitting we need to get it
> upstream and back into the stable kernels ASAP. But this patch
> hasn't even been committed to the dev tree yet...
>
> Can you please get these upstream ASAP? If you don't have the time
> to do it today, let me know so I can send the patches straight to
> Linus and get them to the upstream and stable trees that way....
I pulled these in yesterday. I'll send the pull request this afternoon.
Regards,
Ben
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 0/6] xfs: fixes for 3.7-rc3
2012-11-02 0:38 [PATCH 0/6] xfs: fixes for 3.7-rc3 Dave Chinner
` (6 preceding siblings ...)
2012-11-07 20:56 ` [PATCH 0/6] xfs: fixes for 3.7-rc3 Dave Chinner
@ 2012-11-08 16:15 ` Ben Myers
7 siblings, 0 replies; 33+ messages in thread
From: Ben Myers @ 2012-11-08 16:15 UTC (permalink / raw)
To: Dave Chinner; +Cc: xfs
On Fri, Nov 02, 2012 at 11:38:38AM +1100, Dave Chinner wrote:
> Here are my current outstanding fixes that are candidates for 3.7.
> Patches 2 and 3 are the initial fixes from the verifier series (real
> bugs) and the other are regressions that I've recently fixed. Some
> need to be sent back to stable kernels as well and are tagged as
> such.
Thanks for reposting. Some of these I didn't have in my queue.
I've committed the patches in this seres to git://oss.sgi.com/xfs/xfs.git,
master and for-next branches, with v2 of patch 5. I'll do a pull request asap.
Regards,
Ben
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 33+ messages in thread