xfs: remote attribute blocks aren't really userdata

When adding a new remote attribute, we write the attribute to the
new extent before the allocation transaction is committed. This
means we cannot reuse busy extents as that violates crash
consistency semantics. Hence we currently treat remote attribute
extent allocation like userdata because it has the same overwrite
ordering constraints as userdata.

Unfortunately, this also allows the allocator to incorrectly apply
extent size hints to the remote attribute extent allocation. This
results in interesting failures, such as transaction block
reservation overruns and in-memory inode attribute fork corruption.

To fix this, we need to separate the busy extent reuse configuration
from the userdata configuration. This changes the definition of
XFS_BMAPI_METADATA slightly - it now means that allocation is
metadata and reuse of busy extents is acceptible due to the metadata
ordering semantics of the journal. If this flag is not set, it
means the allocation is that has unordered data writeback, and hence
busy extent reuse is not allowed. It no longer implies the
allocation is for user data, just that the data write will not be
strictly ordered. This matches the semantics for both user data
and remote attribute block allocation.

As such, This patch changes the "userdata" field to a "datatype"
field, and adds a "no busy reuse" flag to the field.
When we detect an unordered data extent allocation, we immediately set
the no reuse flag. We then set the "user data" flags based on the
inode fork we are allocating the extent to. Hence we only set
userdata flags on data fork allocations now and consider attribute
fork remote extents to be an unordered metadata extent.

The result is that remote attribute extents now have the expected
allocation semantics, and the data fork allocation behaviour is
completely unchanged.

It should be noted that there may be other ways to fix this (e.g.
use ordered metadata buffers for the remote attribute extent data
write) but they are more invasive and difficult to validate both
from a design and implementation POV. Hence this patch takes the
simple, obvious route to fixing the problem...

Reported-and-tested-by: Ross Zwisler <ross.zwisler@linux.intel.com>
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Dave Chinner <david@fromorbit.com>

diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index 05b5243d89f6ffcb4d77e71455ef34ec88165ebe..1d530c253c0e2fb471fb4b52675e67491a0a1abc 100644 (file)
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -265,7 +265,7 @@ xfs_alloc_compute_diff(
        xfs_agblock_t   wantbno,        /* target starting block */
        xfs_extlen_t    wantlen,        /* target length */
        xfs_extlen_t    alignment,      /* target alignment */
-       char            userdata,       /* are we allocating data? */
+       int             datatype,       /* are we allocating data? */
        xfs_agblock_t   freebno,        /* freespace's starting block */
        xfs_extlen_t    freelen,        /* freespace's length */
        xfs_agblock_t   *newbnop)       /* result: best start block from free */
@@ -276,6 +276,7 @@ xfs_alloc_compute_diff(
        xfs_extlen_t    newlen1=0;      /* length with newbno1 */
        xfs_extlen_t    newlen2=0;      /* length with newbno2 */
        xfs_agblock_t   wantend;        /* end of target extent */
+       bool            userdata = xfs_alloc_is_userdata(datatype);
 
        ASSERT(freelen >= wantlen);
        freeend = freebno + freelen;
@@ -917,7 +918,7 @@ xfs_alloc_find_best_extent(
 
                        sdiff = xfs_alloc_compute_diff(args->agbno, args->len,
                                                       args->alignment,
-                                                      args->userdata, *sbnoa,
+                                                      args->datatype, *sbnoa,
                                                       *slena, &new);
 
                        /*
@@ -1101,7 +1102,7 @@ restart:
                        if (args->len < blen)
                                continue;
                        ltdiff = xfs_alloc_compute_diff(args->agbno, args->len,
-                               args->alignment, args->userdata, ltbnoa,
+                               args->alignment, args->datatype, ltbnoa,
                                ltlena, &ltnew);
                        if (ltnew != NULLAGBLOCK &&
                            (args->len > blen || ltdiff < bdiff)) {
@@ -1254,7 +1255,7 @@ restart:
                        args->len = XFS_EXTLEN_MIN(ltlena, args->maxlen);
                        xfs_alloc_fix_len(args);
                        ltdiff = xfs_alloc_compute_diff(args->agbno, args->len,
-                               args->alignment, args->userdata, ltbnoa,
+                               args->alignment, args->datatype, ltbnoa,
                                ltlena, &ltnew);
 
                        error = xfs_alloc_find_best_extent(args,
@@ -1271,7 +1272,7 @@ restart:
                        args->len = XFS_EXTLEN_MIN(gtlena, args->maxlen);
                        xfs_alloc_fix_len(args);
                        gtdiff = xfs_alloc_compute_diff(args->agbno, args->len,
-                               args->alignment, args->userdata, gtbnoa,
+                               args->alignment, args->datatype, gtbnoa,
                                gtlena, &gtnew);
 
                        error = xfs_alloc_find_best_extent(args,
@@ -1331,7 +1332,7 @@ restart:
        }
        rlen = args->len;
        (void)xfs_alloc_compute_diff(args->agbno, rlen, args->alignment,
-                                    args->userdata, ltbnoa, ltlena, &ltnew);
+                                    args->datatype, ltbnoa, ltlena, &ltnew);
        ASSERT(ltnew >= ltbno);
        ASSERT(ltnew + rlen <= ltbnoa + ltlena);
        ASSERT(ltnew + rlen <= be32_to_cpu(XFS_BUF_TO_AGF(args->agbp)->agf_length));
@@ -1608,9 +1609,9 @@ xfs_alloc_ag_vextent_small(
                        goto error0;
                if (fbno != NULLAGBLOCK) {
                        xfs_extent_busy_reuse(args->mp, args->agno, fbno, 1,
-                                            args->userdata);
+                             xfs_alloc_allow_busy_reuse(args->datatype));
 
-                       if (args->userdata) {
+                       if (xfs_alloc_is_userdata(args->datatype)) {
                                xfs_buf_t       *bp;
 
                                bp = xfs_btree_get_bufs(args->mp, args->tp,
@@ -2058,7 +2059,7 @@ xfs_alloc_fix_freelist(
         * somewhere else if we are not being asked to try harder at this
         * point
         */
-       if (pag->pagf_metadata && args->userdata &&
+       if (pag->pagf_metadata && xfs_alloc_is_userdata(args->datatype) &&
            (flags & XFS_ALLOC_FLAG_TRYLOCK)) {
                ASSERT(!(flags & XFS_ALLOC_FLAG_FREEING));
                goto out_agbp_relse;
@@ -2633,7 +2634,7 @@ xfs_alloc_vextent(
                 * Try near allocation first, then anywhere-in-ag after
                 * the first a.g. fails.
                 */
-               if ((args->userdata & XFS_ALLOC_INITIAL_USER_DATA) &&
+               if ((args->datatype & XFS_ALLOC_INITIAL_USER_DATA) &&
                    (mp->m_flags & XFS_MOUNT_32BITINODES)) {
                        args->fsbno = XFS_AGB_TO_FSB(mp,
                                        ((mp->m_agfrotor / rotorstep) %
@@ -2766,7 +2767,7 @@ xfs_alloc_vextent(
 #endif
 
                /* Zero the extent if we were asked to do so */
-               if (args->userdata & XFS_ALLOC_USERDATA_ZERO) {
+               if (args->datatype & XFS_ALLOC_USERDATA_ZERO) {
                        error = xfs_zero_extent(args->ip, args->fsbno, args->len);
                        if (error)
                                goto error0;

diff --git a/fs/xfs/libxfs/xfs_alloc.h b/fs/xfs/libxfs/xfs_alloc.h
index 6fe2d6b7cfe93e6ed87f999438877e28ebad3ef6..7fd8eafd9abedb6ce053197fbc3160518bbae451 100644 (file)
--- a/fs/xfs/libxfs/xfs_alloc.h
+++ b/fs/xfs/libxfs/xfs_alloc.h
@@ -85,20 +85,33 @@ typedef struct xfs_alloc_arg {
        xfs_extlen_t    len;            /* output: actual size of extent */
        xfs_alloctype_t type;           /* allocation type XFS_ALLOCTYPE_... */
        xfs_alloctype_t otype;          /* original allocation type */
+       int             datatype;       /* mask defining data type treatment */
        char            wasdel;         /* set if allocation was prev delayed */
        char            wasfromfl;      /* set if allocation is from freelist */
        char            isfl;           /* set if is freelist blocks - !acctg */
-       char            userdata;       /* mask defining userdata treatment */
        xfs_fsblock_t   firstblock;     /* io first block allocated */
        struct xfs_owner_info   oinfo;  /* owner of blocks being allocated */
 } xfs_alloc_arg_t;
 
 /*
- * Defines for userdata
+ * Defines for datatype
  */
 #define XFS_ALLOC_USERDATA             (1 << 0)/* allocation is for user data*/
 #define XFS_ALLOC_INITIAL_USER_DATA    (1 << 1)/* special case start of file */
 #define XFS_ALLOC_USERDATA_ZERO                (1 << 2)/* zero extent on allocation */
+#define XFS_ALLOC_NOBUSY               (1 << 3)/* Busy extents not allowed */
+
+static inline bool
+xfs_alloc_is_userdata(int datatype)
+{
+       return (datatype & ~XFS_ALLOC_NOBUSY) != 0;
+}
+
+static inline bool
+xfs_alloc_allow_busy_reuse(int datatype)
+{
+       return (datatype & XFS_ALLOC_NOBUSY) == 0;
+}
 
 /* freespace limit calculations */
 #define XFS_ALLOC_AGFL_RESERVE 4

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index b060bca93402710fcea1fc2da4544e4d856329bf..06d1201b4718bdf78191906de0241d3ba753d3bb 100644 (file)
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -3347,7 +3347,8 @@ xfs_bmap_adjacent(
 
        mp = ap->ip->i_mount;
        nullfb = *ap->firstblock == NULLFSBLOCK;
-       rt = XFS_IS_REALTIME_INODE(ap->ip) && ap->userdata;
+       rt = XFS_IS_REALTIME_INODE(ap->ip) &&
+               xfs_alloc_is_userdata(ap->datatype);
        fb_agno = nullfb ? NULLAGNUMBER : XFS_FSB_TO_AGNO(mp, *ap->firstblock);
        /*
         * If allocating at eof, and there's a previous real block,
@@ -3622,7 +3623,7 @@ xfs_bmap_btalloc(
 {
        xfs_mount_t     *mp;            /* mount point structure */
        xfs_alloctype_t atype = 0;      /* type for allocation routines */
-       xfs_extlen_t    align;          /* minimum allocation alignment */
+       xfs_extlen_t    align = 0;      /* minimum allocation alignment */
        xfs_agnumber_t  fb_agno;        /* ag number of ap->firstblock */
        xfs_agnumber_t  ag;
        xfs_alloc_arg_t args;
@@ -3645,7 +3646,8 @@ xfs_bmap_btalloc(
        else if (mp->m_dalign)
                stripe_align = mp->m_dalign;
 
-       align = ap->userdata ? xfs_get_extsz_hint(ap->ip) : 0;
+       if (xfs_alloc_is_userdata(ap->datatype))
+               align = xfs_get_extsz_hint(ap->ip);
        if (unlikely(align)) {
                error = xfs_bmap_extsize_align(mp, &ap->got, &ap->prev,
                                                align, 0, ap->eof, 0, ap->conv,
@@ -3658,7 +3660,8 @@ xfs_bmap_btalloc(
        nullfb = *ap->firstblock == NULLFSBLOCK;
        fb_agno = nullfb ? NULLAGNUMBER : XFS_FSB_TO_AGNO(mp, *ap->firstblock);
        if (nullfb) {
-               if (ap->userdata && xfs_inode_is_filestream(ap->ip)) {
+               if (xfs_alloc_is_userdata(ap->datatype) &&
+                   xfs_inode_is_filestream(ap->ip)) {
                        ag = xfs_filestream_lookup_ag(ap->ip);
                        ag = (ag != NULLAGNUMBER) ? ag : 0;
                        ap->blkno = XFS_AGB_TO_FSB(mp, ag, 0);
@@ -3698,7 +3701,8 @@ xfs_bmap_btalloc(
                 * enough for the request.  If one isn't found, then adjust
                 * the minimum allocation size to the largest space found.
                 */
-               if (ap->userdata && xfs_inode_is_filestream(ap->ip))
+               if (xfs_alloc_is_userdata(ap->datatype) &&
+                   xfs_inode_is_filestream(ap->ip))
                        error = xfs_bmap_btalloc_filestreams(ap, &args, &blen);
                else
                        error = xfs_bmap_btalloc_nullfb(ap, &args, &blen);
@@ -3782,8 +3786,8 @@ xfs_bmap_btalloc(
        args.minleft = ap->minleft;
        args.wasdel = ap->wasdel;
        args.isfl = 0;
-       args.userdata = ap->userdata;
-       if (ap->userdata & XFS_ALLOC_USERDATA_ZERO)
+       args.datatype = ap->datatype;
+       if (ap->datatype & XFS_ALLOC_USERDATA_ZERO)
                args.ip = ap->ip;
 
        error = xfs_alloc_vextent(&args);
@@ -3877,7 +3881,8 @@ STATIC int
 xfs_bmap_alloc(
        struct xfs_bmalloca     *ap)    /* bmap alloc argument struct */
 {
-       if (XFS_IS_REALTIME_INODE(ap->ip) && ap->userdata)
+       if (XFS_IS_REALTIME_INODE(ap->ip) &&
+           xfs_alloc_is_userdata(ap->datatype))
                return xfs_bmap_rtalloc(ap);
        return xfs_bmap_btalloc(ap);
 }
@@ -4287,15 +4292,21 @@ xfs_bmapi_allocate(
        }
 
        /*
-        * Indicate if this is the first user data in the file, or just any
-        * user data. And if it is userdata, indicate whether it needs to
-        * be initialised to zero during allocation.
+        * Set the data type being allocated. For the data fork, the first data
+        * in the file is treated differently to all other allocations. For the
+        * attribute fork, we only need to ensure the allocated range is not on
+        * the busy list.
         */
        if (!(bma->flags & XFS_BMAPI_METADATA)) {
-               bma->userdata = (bma->offset == 0) ?
-                       XFS_ALLOC_INITIAL_USER_DATA : XFS_ALLOC_USERDATA;
+               bma->datatype = XFS_ALLOC_NOBUSY;
+               if (whichfork == XFS_DATA_FORK) {
+                       if (bma->offset == 0)
+                               bma->datatype |= XFS_ALLOC_INITIAL_USER_DATA;
+                       else
+                               bma->datatype |= XFS_ALLOC_USERDATA;
+               }
                if (bma->flags & XFS_BMAPI_ZERO)
-                       bma->userdata |= XFS_ALLOC_USERDATA_ZERO;
+                       bma->datatype |= XFS_ALLOC_USERDATA_ZERO;
        }
 
        bma->minlen = (bma->flags & XFS_BMAPI_CONTIG) ? bma->length : 1;
@@ -4565,7 +4576,7 @@ xfs_bmapi_write(
        bma.tp = tp;
        bma.ip = ip;
        bma.total = total;
-       bma.userdata = 0;
+       bma.datatype = 0;
        bma.dfops = dfops;
        bma.firstblock = firstblock;
 

diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
index 254034f9694135c94ad5827c21608f51f183fe3e..05576b7263f6cd3f5fd02ec3cdedaeb59a8d8e89 100644 (file)
--- a/fs/xfs/libxfs/xfs_bmap.h
+++ b/fs/xfs/libxfs/xfs_bmap.h
@@ -54,7 +54,7 @@ struct xfs_bmalloca {
        bool                    wasdel; /* replacing a delayed allocation */
        bool                    aeof;   /* allocated space at eof */
        bool                    conv;   /* overwriting unwritten extents */
-       char                    userdata;/* userdata mask */
+       int                     datatype;/* data type being allocated */
        int                     flags;
 };
 

diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 4ece4f2ffc7271ef7249b4e410f5e48391aa0383..e827d657c3145cb65d90725a476ce719b2a6c2ac 100644 (file)
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -182,7 +182,7 @@ xfs_bmap_rtalloc(
                                        XFS_TRANS_DQ_RTBCOUNT, (long) ralen);
 
                /* Zero the extent if we were asked to do so */
-               if (ap->userdata & XFS_ALLOC_USERDATA_ZERO) {
+               if (ap->datatype & XFS_ALLOC_USERDATA_ZERO) {
                        error = xfs_zero_extent(ap->ip, ap->blkno, ap->length);
                        if (error)
                                return error;

diff --git a/fs/xfs/xfs_extent_busy.c b/fs/xfs/xfs_extent_busy.c
index c263e079273e445d61006ce7d01a436d8467c3d3..162dc186cf045c38ee31288e87025e7879eb039e 100644 (file)
--- a/fs/xfs/xfs_extent_busy.c
+++ b/fs/xfs/xfs_extent_busy.c
@@ -384,7 +384,7 @@ restart:
                 * If this is a metadata allocation, try to reuse the busy
                 * extent instead of trimming the allocation.
                 */
-               if (!args->userdata &&
+               if (!xfs_alloc_is_userdata(args->datatype) &&
                    !(busyp->flags & XFS_EXTENT_BUSY_DISCARDED)) {
                        if (!xfs_extent_busy_update_extent(args->mp, args->pag,
                                                          busyp, fbno, flen,

diff --git a/fs/xfs/xfs_filestream.c b/fs/xfs/xfs_filestream.c
index 4a33a3304369109f2864bf97362962865f56c930..a75f7ab0581c89b5e98d9b7ee502e4c57803b61f 100644 (file)
--- a/fs/xfs/xfs_filestream.c
+++ b/fs/xfs/xfs_filestream.c
@@ -369,7 +369,8 @@ xfs_filestream_new_ag(
        struct xfs_mount        *mp = ip->i_mount;
        xfs_extlen_t            minlen = ap->length;
        xfs_agnumber_t          startag = 0;
-       int                     flags, err = 0;
+       int                     flags = 0;
+       int                     err = 0;
        struct xfs_mru_cache_elem *mru;
 
        *agp = NULLAGNUMBER;
@@ -385,8 +386,10 @@ xfs_filestream_new_ag(
                startag = (item->ag + 1) % mp->m_sb.sb_agcount;
        }
 
-       flags = (ap->userdata ? XFS_PICK_USERDATA : 0) |
-               (ap->dfops->dop_low ? XFS_PICK_LOWSPACE : 0);
+       if (xfs_alloc_is_userdata(ap->datatype))
+               flags |= XFS_PICK_USERDATA;
+       if (ap->dfops->dop_low)
+               flags |= XFS_PICK_LOWSPACE;
 
        err = xfs_filestream_pick_ag(pip, startag, agp, flags, minlen);
 

diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index d303a665dba9df637026e36709a962e261792429..1144522799bb34abe1a064c2c37923b2256d0535 100644 (file)
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -1623,7 +1623,7 @@ DECLARE_EVENT_CLASS(xfs_alloc_class,
                __field(char, wasdel)
                __field(char, wasfromfl)
                __field(char, isfl)
-               __field(char, userdata)
+               __field(int, datatype)
                __field(xfs_fsblock_t, firstblock)
        ),
        TP_fast_assign(
@@ -1644,13 +1644,13 @@ DECLARE_EVENT_CLASS(xfs_alloc_class,
                __entry->wasdel = args->wasdel;
                __entry->wasfromfl = args->wasfromfl;
                __entry->isfl = args->isfl;
-               __entry->userdata = args->userdata;
+               __entry->datatype = args->datatype;
                __entry->firstblock = args->firstblock;
        ),
        TP_printk("dev %d:%d agno %u agbno %u minlen %u maxlen %u mod %u "
                  "prod %u minleft %u total %u alignment %u minalignslop %u "
                  "len %u type %s otype %s wasdel %d wasfromfl %d isfl %d "
-                 "userdata %d firstblock 0x%llx",
+                 "datatype 0x%x firstblock 0x%llx",
                  MAJOR(__entry->dev), MINOR(__entry->dev),
                  __entry->agno,
                  __entry->agbno,
@@ -1668,7 +1668,7 @@ DECLARE_EVENT_CLASS(xfs_alloc_class,
                  __entry->wasdel,
                  __entry->wasfromfl,
                  __entry->isfl,
-                 __entry->userdata,
+                 __entry->datatype,
                  (unsigned long long)__entry->firstblock)
 )