* a few sparse inspired cleanups / fixes
@ 2017-04-21 15:21 Christoph Hellwig
2017-04-21 15:21 ` [PATCH 1/6] xfs: use NULL instead of 0 to initialize a pointer in xfs_getfsmap Christoph Hellwig
` (5 more replies)
0 siblings, 6 replies; 14+ messages in thread
From: Christoph Hellwig @ 2017-04-21 15:21 UTC (permalink / raw)
To: linux-xfs
Starting from trivial to more intrusive near the end.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/6] xfs: use NULL instead of 0 to initialize a pointer in xfs_getfsmap
2017-04-21 15:21 a few sparse inspired cleanups / fixes Christoph Hellwig
@ 2017-04-21 15:21 ` Christoph Hellwig
2017-04-21 19:04 ` Darrick J. Wong
2017-04-21 15:21 ` [PATCH 2/6] xfs: use NULL instead of 0 to initialize a pointer in xfs_ioc_getfsmap Christoph Hellwig
` (4 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2017-04-21 15:21 UTC (permalink / raw)
To: linux-xfs
Found by sparse.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_fsmap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
index 48ce356da9e9..3683819887a5 100644
--- a/fs/xfs/xfs_fsmap.c
+++ b/fs/xfs/xfs_fsmap.c
@@ -827,7 +827,7 @@ xfs_getfsmap(
struct xfs_trans *tp = NULL;
struct xfs_fsmap dkeys[2]; /* per-dev keys */
struct xfs_getfsmap_dev handlers[XFS_GETFSMAP_DEVS];
- struct xfs_getfsmap_info info = {0};
+ struct xfs_getfsmap_info info = { NULL };
int i;
int error = 0;
--
2.11.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/6] xfs: use NULL instead of 0 to initialize a pointer in xfs_ioc_getfsmap
2017-04-21 15:21 a few sparse inspired cleanups / fixes Christoph Hellwig
2017-04-21 15:21 ` [PATCH 1/6] xfs: use NULL instead of 0 to initialize a pointer in xfs_getfsmap Christoph Hellwig
@ 2017-04-21 15:21 ` Christoph Hellwig
2017-04-21 19:04 ` Darrick J. Wong
2017-04-21 15:21 ` [PATCH 3/6] xfs: corruption needs to respect endianess too! Christoph Hellwig
` (3 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2017-04-21 15:21 UTC (permalink / raw)
To: linux-xfs
Found by sparse.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
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 fee6b4a3a83d..0f8bed9a7e4c 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1640,7 +1640,7 @@ xfs_ioc_getfsmap(
struct xfs_inode *ip,
void __user *arg)
{
- struct getfsmap_info info = {0};
+ struct getfsmap_info info = { NULL };
struct xfs_fsmap_head xhead = {0};
struct fsmap_head head;
bool aborted = false;
--
2.11.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/6] xfs: corruption needs to respect endianess too!
2017-04-21 15:21 a few sparse inspired cleanups / fixes Christoph Hellwig
2017-04-21 15:21 ` [PATCH 1/6] xfs: use NULL instead of 0 to initialize a pointer in xfs_getfsmap Christoph Hellwig
2017-04-21 15:21 ` [PATCH 2/6] xfs: use NULL instead of 0 to initialize a pointer in xfs_ioc_getfsmap Christoph Hellwig
@ 2017-04-21 15:21 ` Christoph Hellwig
2017-04-21 19:05 ` Darrick J. Wong
2017-04-21 15:21 ` [PATCH 4/6] xfs: fix __user annotations for xfs_ioc_getfsmap Christoph Hellwig
` (2 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2017-04-21 15:21 UTC (permalink / raw)
To: linux-xfs
At least if we want to be able to recognize the pattern. Add a missing
byte swap to the corruption injection case in xlog_sync.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_log.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index bb58cd1873c9..3731f13f63e9 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -1852,7 +1852,7 @@ xlog_sync(
*/
if (log->l_badcrc_factor &&
(prandom_u32() % log->l_badcrc_factor == 0)) {
- iclog->ic_header.h_crc &= 0xAAAAAAAA;
+ iclog->ic_header.h_crc &= cpu_to_le32(0xAAAAAAAA);
iclog->ic_state |= XLOG_STATE_IOABORT;
xfs_warn(log->l_mp,
"Intentionally corrupted log record at LSN 0x%llx. Shutdown imminent.",
--
2.11.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 4/6] xfs: fix __user annotations for xfs_ioc_getfsmap
2017-04-21 15:21 a few sparse inspired cleanups / fixes Christoph Hellwig
` (2 preceding siblings ...)
2017-04-21 15:21 ` [PATCH 3/6] xfs: corruption needs to respect endianess too! Christoph Hellwig
@ 2017-04-21 15:21 ` Christoph Hellwig
2017-04-21 19:08 ` Darrick J. Wong
2017-04-21 15:21 ` [PATCH 5/6] xfs: don't use bool values in trace buffers Christoph Hellwig
2017-04-21 15:21 ` [PATCH 6/6] xfs: remove xfs_trans_ail_delete_bulk Christoph Hellwig
5 siblings, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2017-04-21 15:21 UTC (permalink / raw)
To: linux-xfs
By passing the whole fsmap_head structure and an index we can get the
user point annotations right for the embedded variable sized array
in struct fsmap_head.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_ioctl.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 0f8bed9a7e4c..b7db0b8f0657 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1614,7 +1614,8 @@ xfs_ioc_getbmapx(
struct getfsmap_info {
struct xfs_mount *mp;
- struct fsmap __user *data;
+ struct fsmap_head __user *data;
+ int idx;
__u32 last_flags;
};
@@ -1628,17 +1629,17 @@ xfs_getfsmap_format(struct xfs_fsmap *xfm, void *priv)
info->last_flags = xfm->fmr_flags;
xfs_fsmap_from_internal(&fm, xfm);
- if (copy_to_user(info->data, &fm, sizeof(struct fsmap)))
+ if (copy_to_user(&info->data->fmh_recs[info->idx++], &fm,
+ sizeof(struct fsmap)))
return -EFAULT;
- info->data++;
return 0;
}
STATIC int
xfs_ioc_getfsmap(
struct xfs_inode *ip,
- void __user *arg)
+ struct fsmap_head __user *arg)
{
struct getfsmap_info info = { NULL };
struct xfs_fsmap_head xhead = {0};
@@ -1664,7 +1665,7 @@ xfs_ioc_getfsmap(
trace_xfs_getfsmap_high_key(ip->i_mount, &xhead.fmh_keys[1]);
info.mp = ip->i_mount;
- info.data = ((__force struct fsmap_head *)arg)->fmh_recs;
+ info.data = arg;
error = xfs_getfsmap(ip->i_mount, &xhead, xfs_getfsmap_format, &info);
if (error == XFS_BTREE_QUERY_RANGE_ABORT) {
error = 0;
@@ -1674,10 +1675,9 @@ xfs_ioc_getfsmap(
/* If we didn't abort, set the "last" flag in the last fmx */
if (!aborted && xhead.fmh_entries) {
- info.data--;
info.last_flags |= FMR_OF_LAST;
- if (copy_to_user(&info.data->fmr_flags, &info.last_flags,
- sizeof(info.last_flags)))
+ if (copy_to_user(&info.data->fmh_recs[info.idx - 1].fmr_flags,
+ &info.last_flags, sizeof(info.last_flags)))
return -EFAULT;
}
--
2.11.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 5/6] xfs: don't use bool values in trace buffers
2017-04-21 15:21 a few sparse inspired cleanups / fixes Christoph Hellwig
` (3 preceding siblings ...)
2017-04-21 15:21 ` [PATCH 4/6] xfs: fix __user annotations for xfs_ioc_getfsmap Christoph Hellwig
@ 2017-04-21 15:21 ` Christoph Hellwig
2017-04-21 19:33 ` Darrick J. Wong
2017-04-21 15:21 ` [PATCH 6/6] xfs: remove xfs_trans_ail_delete_bulk Christoph Hellwig
5 siblings, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2017-04-21 15:21 UTC (permalink / raw)
To: linux-xfs
Using bool values produces sparse warnings of this form:
fs/xfs/./xfs_trace.h:2252:1: warning: odd constant _Bool cast (ffffffffffffffff becomes 1)
fs/xfs/./xfs_trace.h:2252:1: warning: odd constant _Bool cast (ffffffffffffffff becomes 1)
fs/xfs/./xfs_trace.h:2278:1: warning: odd constant _Bool cast (ffffffffffffffff becomes 1)
fs/xfs/./xfs_trace.h:2278:1: warning: odd constant _Bool cast (ffffffffffffffff becomes 1)
fs/xfs/./xfs_trace.h:2307:1: warning: odd constant _Bool cast (ffffffffffffffff becomes 1)
Just use a char instead to fix those up.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_trace.h | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index 4f96dc953fbe..250d08b7cfba 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -2255,8 +2255,8 @@ DECLARE_EVENT_CLASS(xfs_defer_class,
TP_STRUCT__entry(
__field(dev_t, dev)
__field(void *, dop)
- __field(bool, committed)
- __field(bool, low)
+ __field(char, committed)
+ __field(char, low)
),
TP_fast_assign(
__entry->dev = mp ? mp->m_super->s_dev : 0;
@@ -2281,8 +2281,8 @@ DECLARE_EVENT_CLASS(xfs_defer_error_class,
TP_STRUCT__entry(
__field(dev_t, dev)
__field(void *, dop)
- __field(bool, committed)
- __field(bool, low)
+ __field(char, committed)
+ __field(char, low)
__field(int, error)
),
TP_fast_assign(
@@ -2311,7 +2311,7 @@ DECLARE_EVENT_CLASS(xfs_defer_pending_class,
__field(dev_t, dev)
__field(int, type)
__field(void *, intent)
- __field(bool, committed)
+ __field(char, committed)
__field(int, nr)
),
TP_fast_assign(
--
2.11.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 6/6] xfs: remove xfs_trans_ail_delete_bulk
2017-04-21 15:21 a few sparse inspired cleanups / fixes Christoph Hellwig
` (4 preceding siblings ...)
2017-04-21 15:21 ` [PATCH 5/6] xfs: don't use bool values in trace buffers Christoph Hellwig
@ 2017-04-21 15:21 ` Christoph Hellwig
2017-04-21 19:35 ` Darrick J. Wong
5 siblings, 1 reply; 14+ messages in thread
From: Christoph Hellwig @ 2017-04-21 15:21 UTC (permalink / raw)
To: linux-xfs
xfs_iflush_done uses an on-stack variable length array to pass the log
items to be deleted to xfs_trans_ail_delete_bulk. On-stack VLAs are a
nasty gcc extension that can lead to unbounded stack allocations, but
fortunately we can easily avoid them by simply open coding
xfs_trans_ail_delete_bulk in xfs_iflush_done, which is the only caller
of it except for the single-item xfs_trans_ail_delete.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_inode_item.c | 29 +++++++++++---------
fs/xfs/xfs_trans_ail.c | 71 ++++++++++++++++++++++++-------------------------
fs/xfs/xfs_trans_priv.h | 15 +++--------
3 files changed, 55 insertions(+), 60 deletions(-)
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index d90e7811ccdd..08cb7d1a4a3a 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -731,22 +731,27 @@ xfs_iflush_done(
* holding the lock before removing the inode from the AIL.
*/
if (need_ail) {
- struct xfs_log_item *log_items[need_ail];
- int i = 0;
+ bool mlip_changed = false;
+
+ /* this is an opencoded batch version of xfs_trans_ail_delete */
spin_lock(&ailp->xa_lock);
for (blip = lip; blip; blip = blip->li_bio_list) {
- iip = INODE_ITEM(blip);
- if (iip->ili_logged &&
- blip->li_lsn == iip->ili_flush_lsn) {
- log_items[i++] = blip;
- }
- ASSERT(i <= need_ail);
+ if (INODE_ITEM(blip)->ili_logged &&
+ blip->li_lsn == INODE_ITEM(blip)->ili_flush_lsn)
+ mlip_changed |= xfs_ail_delete_one(ailp, blip);
}
- /* xfs_trans_ail_delete_bulk() drops the AIL lock. */
- xfs_trans_ail_delete_bulk(ailp, log_items, i,
- SHUTDOWN_CORRUPT_INCORE);
- }
+ if (mlip_changed) {
+ if (!XFS_FORCED_SHUTDOWN(ailp->xa_mount))
+ xlog_assign_tail_lsn_locked(ailp->xa_mount);
+ if (list_empty(&ailp->xa_ail))
+ wake_up_all(&ailp->xa_empty);
+ }
+ spin_unlock(&ailp->xa_lock);
+
+ if (mlip_changed)
+ xfs_log_space_wake(ailp->xa_mount);
+ }
/*
* clean up and unlock the flush lock now we are done. We can clear the
diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index d6c9c3e9e02b..9056c0f34a3c 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -684,8 +684,23 @@ xfs_trans_ail_update_bulk(
}
}
-/*
- * xfs_trans_ail_delete_bulk - remove multiple log items from the AIL
+bool
+xfs_ail_delete_one(
+ struct xfs_ail *ailp,
+ struct xfs_log_item *lip)
+{
+ struct xfs_log_item *mlip = xfs_ail_min(ailp);
+
+ trace_xfs_ail_delete(lip, mlip->li_lsn, lip->li_lsn);
+ xfs_ail_delete(ailp, lip);
+ lip->li_flags &= ~XFS_LI_IN_AIL;
+ lip->li_lsn = 0;
+
+ return mlip == lip;
+}
+
+/**
+ * Remove a log items from the AIL
*
* @xfs_trans_ail_delete_bulk takes an array of log items that all need to
* removed from the AIL. The caller is already holding the AIL lock, and done
@@ -706,52 +721,36 @@ xfs_trans_ail_update_bulk(
* before returning.
*/
void
-xfs_trans_ail_delete_bulk(
+xfs_trans_ail_delete(
struct xfs_ail *ailp,
- struct xfs_log_item **log_items,
- int nr_items,
+ struct xfs_log_item *lip,
int shutdown_type) __releases(ailp->xa_lock)
{
- xfs_log_item_t *mlip;
- int mlip_changed = 0;
- int i;
+ struct xfs_mount *mp = ailp->xa_mount;
+ bool mlip_changed;
- mlip = xfs_ail_min(ailp);
-
- for (i = 0; i < nr_items; i++) {
- struct xfs_log_item *lip = log_items[i];
- if (!(lip->li_flags & XFS_LI_IN_AIL)) {
- struct xfs_mount *mp = ailp->xa_mount;
-
- spin_unlock(&ailp->xa_lock);
- if (!XFS_FORCED_SHUTDOWN(mp)) {
- xfs_alert_tag(mp, XFS_PTAG_AILDELETE,
- "%s: attempting to delete a log item that is not in the AIL",
- __func__);
- xfs_force_shutdown(mp, shutdown_type);
- }
- return;
+ if (!(lip->li_flags & XFS_LI_IN_AIL)) {
+ spin_unlock(&ailp->xa_lock);
+ if (!XFS_FORCED_SHUTDOWN(mp)) {
+ xfs_alert_tag(mp, XFS_PTAG_AILDELETE,
+ "%s: attempting to delete a log item that is not in the AIL",
+ __func__);
+ xfs_force_shutdown(mp, shutdown_type);
}
-
- trace_xfs_ail_delete(lip, mlip->li_lsn, lip->li_lsn);
- xfs_ail_delete(ailp, lip);
- lip->li_flags &= ~XFS_LI_IN_AIL;
- lip->li_lsn = 0;
- if (mlip == lip)
- mlip_changed = 1;
+ return;
}
+ mlip_changed = xfs_ail_delete_one(ailp, lip);
if (mlip_changed) {
- if (!XFS_FORCED_SHUTDOWN(ailp->xa_mount))
- xlog_assign_tail_lsn_locked(ailp->xa_mount);
+ if (!XFS_FORCED_SHUTDOWN(mp))
+ xlog_assign_tail_lsn_locked(mp);
if (list_empty(&ailp->xa_ail))
wake_up_all(&ailp->xa_empty);
- spin_unlock(&ailp->xa_lock);
+ }
+ spin_unlock(&ailp->xa_lock);
+ if (mlip_changed)
xfs_log_space_wake(ailp->xa_mount);
- } else {
- spin_unlock(&ailp->xa_lock);
- }
}
int
diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
index 49931b72da8a..d91706c56c63 100644
--- a/fs/xfs/xfs_trans_priv.h
+++ b/fs/xfs/xfs_trans_priv.h
@@ -106,18 +106,9 @@ xfs_trans_ail_update(
xfs_trans_ail_update_bulk(ailp, NULL, &lip, 1, lsn);
}
-void xfs_trans_ail_delete_bulk(struct xfs_ail *ailp,
- struct xfs_log_item **log_items, int nr_items,
- int shutdown_type)
- __releases(ailp->xa_lock);
-static inline void
-xfs_trans_ail_delete(
- struct xfs_ail *ailp,
- xfs_log_item_t *lip,
- int shutdown_type) __releases(ailp->xa_lock)
-{
- xfs_trans_ail_delete_bulk(ailp, &lip, 1, shutdown_type);
-}
+bool xfs_ail_delete_one(struct xfs_ail *ailp, struct xfs_log_item *lip);
+void xfs_trans_ail_delete(struct xfs_ail *ailp, struct xfs_log_item *lip,
+ int shutdown_type) __releases(ailp->xa_lock);
static inline void
xfs_trans_ail_remove(
--
2.11.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/6] xfs: use NULL instead of 0 to initialize a pointer in xfs_getfsmap
2017-04-21 15:21 ` [PATCH 1/6] xfs: use NULL instead of 0 to initialize a pointer in xfs_getfsmap Christoph Hellwig
@ 2017-04-21 19:04 ` Darrick J. Wong
0 siblings, 0 replies; 14+ messages in thread
From: Darrick J. Wong @ 2017-04-21 19:04 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs
On Fri, Apr 21, 2017 at 05:21:18PM +0200, Christoph Hellwig wrote:
> Found by sparse.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/xfs_fsmap.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
> index 48ce356da9e9..3683819887a5 100644
> --- a/fs/xfs/xfs_fsmap.c
> +++ b/fs/xfs/xfs_fsmap.c
> @@ -827,7 +827,7 @@ xfs_getfsmap(
> struct xfs_trans *tp = NULL;
> struct xfs_fsmap dkeys[2]; /* per-dev keys */
> struct xfs_getfsmap_dev handlers[XFS_GETFSMAP_DEVS];
> - struct xfs_getfsmap_info info = {0};
> + struct xfs_getfsmap_info info = { NULL };
> int i;
> int error = 0;
>
> --
> 2.11.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/6] xfs: use NULL instead of 0 to initialize a pointer in xfs_ioc_getfsmap
2017-04-21 15:21 ` [PATCH 2/6] xfs: use NULL instead of 0 to initialize a pointer in xfs_ioc_getfsmap Christoph Hellwig
@ 2017-04-21 19:04 ` Darrick J. Wong
0 siblings, 0 replies; 14+ messages in thread
From: Darrick J. Wong @ 2017-04-21 19:04 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs
On Fri, Apr 21, 2017 at 05:21:19PM +0200, Christoph Hellwig wrote:
> Found by sparse.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> 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 fee6b4a3a83d..0f8bed9a7e4c 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1640,7 +1640,7 @@ xfs_ioc_getfsmap(
> struct xfs_inode *ip,
> void __user *arg)
> {
> - struct getfsmap_info info = {0};
> + struct getfsmap_info info = { NULL };
> struct xfs_fsmap_head xhead = {0};
> struct fsmap_head head;
> bool aborted = false;
> --
> 2.11.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/6] xfs: corruption needs to respect endianess too!
2017-04-21 15:21 ` [PATCH 3/6] xfs: corruption needs to respect endianess too! Christoph Hellwig
@ 2017-04-21 19:05 ` Darrick J. Wong
0 siblings, 0 replies; 14+ messages in thread
From: Darrick J. Wong @ 2017-04-21 19:05 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs
On Fri, Apr 21, 2017 at 05:21:20PM +0200, Christoph Hellwig wrote:
> At least if we want to be able to recognize the pattern. Add a missing
> byte swap to the corruption injection case in xlog_sync.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/xfs_log.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index bb58cd1873c9..3731f13f63e9 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -1852,7 +1852,7 @@ xlog_sync(
> */
> if (log->l_badcrc_factor &&
> (prandom_u32() % log->l_badcrc_factor == 0)) {
> - iclog->ic_header.h_crc &= 0xAAAAAAAA;
> + iclog->ic_header.h_crc &= cpu_to_le32(0xAAAAAAAA);
> iclog->ic_state |= XLOG_STATE_IOABORT;
> xfs_warn(log->l_mp,
> "Intentionally corrupted log record at LSN 0x%llx. Shutdown imminent.",
> --
> 2.11.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/6] xfs: fix __user annotations for xfs_ioc_getfsmap
2017-04-21 15:21 ` [PATCH 4/6] xfs: fix __user annotations for xfs_ioc_getfsmap Christoph Hellwig
@ 2017-04-21 19:08 ` Darrick J. Wong
0 siblings, 0 replies; 14+ messages in thread
From: Darrick J. Wong @ 2017-04-21 19:08 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs
On Fri, Apr 21, 2017 at 05:21:21PM +0200, Christoph Hellwig wrote:
> By passing the whole fsmap_head structure and an index we can get the
> user point annotations right for the embedded variable sized array
> in struct fsmap_head.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/xfs/xfs_ioctl.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 0f8bed9a7e4c..b7db0b8f0657 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1614,7 +1614,8 @@ xfs_ioc_getbmapx(
>
> struct getfsmap_info {
> struct xfs_mount *mp;
> - struct fsmap __user *data;
> + struct fsmap_head __user *data;
> + int idx;
This ought to be unsigned int since fmh_entries is u32. I'll fix
it when I commit the patch.
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> __u32 last_flags;
> };
>
> @@ -1628,17 +1629,17 @@ xfs_getfsmap_format(struct xfs_fsmap *xfm, void *priv)
>
> info->last_flags = xfm->fmr_flags;
> xfs_fsmap_from_internal(&fm, xfm);
> - if (copy_to_user(info->data, &fm, sizeof(struct fsmap)))
> + if (copy_to_user(&info->data->fmh_recs[info->idx++], &fm,
> + sizeof(struct fsmap)))
> return -EFAULT;
>
> - info->data++;
> return 0;
> }
>
> STATIC int
> xfs_ioc_getfsmap(
> struct xfs_inode *ip,
> - void __user *arg)
> + struct fsmap_head __user *arg)
> {
> struct getfsmap_info info = { NULL };
> struct xfs_fsmap_head xhead = {0};
> @@ -1664,7 +1665,7 @@ xfs_ioc_getfsmap(
> trace_xfs_getfsmap_high_key(ip->i_mount, &xhead.fmh_keys[1]);
>
> info.mp = ip->i_mount;
> - info.data = ((__force struct fsmap_head *)arg)->fmh_recs;
> + info.data = arg;
> error = xfs_getfsmap(ip->i_mount, &xhead, xfs_getfsmap_format, &info);
> if (error == XFS_BTREE_QUERY_RANGE_ABORT) {
> error = 0;
> @@ -1674,10 +1675,9 @@ xfs_ioc_getfsmap(
>
> /* If we didn't abort, set the "last" flag in the last fmx */
> if (!aborted && xhead.fmh_entries) {
> - info.data--;
> info.last_flags |= FMR_OF_LAST;
> - if (copy_to_user(&info.data->fmr_flags, &info.last_flags,
> - sizeof(info.last_flags)))
> + if (copy_to_user(&info.data->fmh_recs[info.idx - 1].fmr_flags,
> + &info.last_flags, sizeof(info.last_flags)))
> return -EFAULT;
> }
>
> --
> 2.11.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 5/6] xfs: don't use bool values in trace buffers
2017-04-21 15:21 ` [PATCH 5/6] xfs: don't use bool values in trace buffers Christoph Hellwig
@ 2017-04-21 19:33 ` Darrick J. Wong
2017-04-23 7:38 ` Christoph Hellwig
0 siblings, 1 reply; 14+ messages in thread
From: Darrick J. Wong @ 2017-04-21 19:33 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs
On Fri, Apr 21, 2017 at 05:21:22PM +0200, Christoph Hellwig wrote:
> Using bool values produces sparse warnings of this form:
>
> fs/xfs/./xfs_trace.h:2252:1: warning: odd constant _Bool cast (ffffffffffffffff becomes 1)
> fs/xfs/./xfs_trace.h:2252:1: warning: odd constant _Bool cast (ffffffffffffffff becomes 1)
> fs/xfs/./xfs_trace.h:2278:1: warning: odd constant _Bool cast (ffffffffffffffff becomes 1)
> fs/xfs/./xfs_trace.h:2278:1: warning: odd constant _Bool cast (ffffffffffffffff becomes 1)
> fs/xfs/./xfs_trace.h:2307:1: warning: odd constant _Bool cast (ffffffffffffffff becomes 1)
>
> Just use a char instead to fix those up.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
/me wonders what version of sparse produces this?
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/xfs_trace.h | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index 4f96dc953fbe..250d08b7cfba 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -2255,8 +2255,8 @@ DECLARE_EVENT_CLASS(xfs_defer_class,
> TP_STRUCT__entry(
> __field(dev_t, dev)
> __field(void *, dop)
> - __field(bool, committed)
> - __field(bool, low)
> + __field(char, committed)
> + __field(char, low)
> ),
> TP_fast_assign(
> __entry->dev = mp ? mp->m_super->s_dev : 0;
> @@ -2281,8 +2281,8 @@ DECLARE_EVENT_CLASS(xfs_defer_error_class,
> TP_STRUCT__entry(
> __field(dev_t, dev)
> __field(void *, dop)
> - __field(bool, committed)
> - __field(bool, low)
> + __field(char, committed)
> + __field(char, low)
> __field(int, error)
> ),
> TP_fast_assign(
> @@ -2311,7 +2311,7 @@ DECLARE_EVENT_CLASS(xfs_defer_pending_class,
> __field(dev_t, dev)
> __field(int, type)
> __field(void *, intent)
> - __field(bool, committed)
> + __field(char, committed)
> __field(int, nr)
> ),
> TP_fast_assign(
> --
> 2.11.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 6/6] xfs: remove xfs_trans_ail_delete_bulk
2017-04-21 15:21 ` [PATCH 6/6] xfs: remove xfs_trans_ail_delete_bulk Christoph Hellwig
@ 2017-04-21 19:35 ` Darrick J. Wong
0 siblings, 0 replies; 14+ messages in thread
From: Darrick J. Wong @ 2017-04-21 19:35 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-xfs
On Fri, Apr 21, 2017 at 05:21:23PM +0200, Christoph Hellwig wrote:
> xfs_iflush_done uses an on-stack variable length array to pass the log
> items to be deleted to xfs_trans_ail_delete_bulk. On-stack VLAs are a
> nasty gcc extension that can lead to unbounded stack allocations, but
> fortunately we can easily avoid them by simply open coding
> xfs_trans_ail_delete_bulk in xfs_iflush_done, which is the only caller
> of it except for the single-item xfs_trans_ail_delete.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Looks ok, will throw it on the testing pile...
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/xfs_inode_item.c | 29 +++++++++++---------
> fs/xfs/xfs_trans_ail.c | 71 ++++++++++++++++++++++++-------------------------
> fs/xfs/xfs_trans_priv.h | 15 +++--------
> 3 files changed, 55 insertions(+), 60 deletions(-)
>
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index d90e7811ccdd..08cb7d1a4a3a 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -731,22 +731,27 @@ xfs_iflush_done(
> * holding the lock before removing the inode from the AIL.
> */
> if (need_ail) {
> - struct xfs_log_item *log_items[need_ail];
> - int i = 0;
> + bool mlip_changed = false;
> +
> + /* this is an opencoded batch version of xfs_trans_ail_delete */
> spin_lock(&ailp->xa_lock);
> for (blip = lip; blip; blip = blip->li_bio_list) {
> - iip = INODE_ITEM(blip);
> - if (iip->ili_logged &&
> - blip->li_lsn == iip->ili_flush_lsn) {
> - log_items[i++] = blip;
> - }
> - ASSERT(i <= need_ail);
> + if (INODE_ITEM(blip)->ili_logged &&
> + blip->li_lsn == INODE_ITEM(blip)->ili_flush_lsn)
> + mlip_changed |= xfs_ail_delete_one(ailp, blip);
> }
> - /* xfs_trans_ail_delete_bulk() drops the AIL lock. */
> - xfs_trans_ail_delete_bulk(ailp, log_items, i,
> - SHUTDOWN_CORRUPT_INCORE);
> - }
>
> + if (mlip_changed) {
> + if (!XFS_FORCED_SHUTDOWN(ailp->xa_mount))
> + xlog_assign_tail_lsn_locked(ailp->xa_mount);
> + if (list_empty(&ailp->xa_ail))
> + wake_up_all(&ailp->xa_empty);
> + }
> + spin_unlock(&ailp->xa_lock);
> +
> + if (mlip_changed)
> + xfs_log_space_wake(ailp->xa_mount);
> + }
>
> /*
> * clean up and unlock the flush lock now we are done. We can clear the
> diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> index d6c9c3e9e02b..9056c0f34a3c 100644
> --- a/fs/xfs/xfs_trans_ail.c
> +++ b/fs/xfs/xfs_trans_ail.c
> @@ -684,8 +684,23 @@ xfs_trans_ail_update_bulk(
> }
> }
>
> -/*
> - * xfs_trans_ail_delete_bulk - remove multiple log items from the AIL
> +bool
> +xfs_ail_delete_one(
> + struct xfs_ail *ailp,
> + struct xfs_log_item *lip)
> +{
> + struct xfs_log_item *mlip = xfs_ail_min(ailp);
> +
> + trace_xfs_ail_delete(lip, mlip->li_lsn, lip->li_lsn);
> + xfs_ail_delete(ailp, lip);
> + lip->li_flags &= ~XFS_LI_IN_AIL;
> + lip->li_lsn = 0;
> +
> + return mlip == lip;
> +}
> +
> +/**
> + * Remove a log items from the AIL
> *
> * @xfs_trans_ail_delete_bulk takes an array of log items that all need to
> * removed from the AIL. The caller is already holding the AIL lock, and done
> @@ -706,52 +721,36 @@ xfs_trans_ail_update_bulk(
> * before returning.
> */
> void
> -xfs_trans_ail_delete_bulk(
> +xfs_trans_ail_delete(
> struct xfs_ail *ailp,
> - struct xfs_log_item **log_items,
> - int nr_items,
> + struct xfs_log_item *lip,
> int shutdown_type) __releases(ailp->xa_lock)
> {
> - xfs_log_item_t *mlip;
> - int mlip_changed = 0;
> - int i;
> + struct xfs_mount *mp = ailp->xa_mount;
> + bool mlip_changed;
>
> - mlip = xfs_ail_min(ailp);
> -
> - for (i = 0; i < nr_items; i++) {
> - struct xfs_log_item *lip = log_items[i];
> - if (!(lip->li_flags & XFS_LI_IN_AIL)) {
> - struct xfs_mount *mp = ailp->xa_mount;
> -
> - spin_unlock(&ailp->xa_lock);
> - if (!XFS_FORCED_SHUTDOWN(mp)) {
> - xfs_alert_tag(mp, XFS_PTAG_AILDELETE,
> - "%s: attempting to delete a log item that is not in the AIL",
> - __func__);
> - xfs_force_shutdown(mp, shutdown_type);
> - }
> - return;
> + if (!(lip->li_flags & XFS_LI_IN_AIL)) {
> + spin_unlock(&ailp->xa_lock);
> + if (!XFS_FORCED_SHUTDOWN(mp)) {
> + xfs_alert_tag(mp, XFS_PTAG_AILDELETE,
> + "%s: attempting to delete a log item that is not in the AIL",
> + __func__);
> + xfs_force_shutdown(mp, shutdown_type);
> }
> -
> - trace_xfs_ail_delete(lip, mlip->li_lsn, lip->li_lsn);
> - xfs_ail_delete(ailp, lip);
> - lip->li_flags &= ~XFS_LI_IN_AIL;
> - lip->li_lsn = 0;
> - if (mlip == lip)
> - mlip_changed = 1;
> + return;
> }
>
> + mlip_changed = xfs_ail_delete_one(ailp, lip);
> if (mlip_changed) {
> - if (!XFS_FORCED_SHUTDOWN(ailp->xa_mount))
> - xlog_assign_tail_lsn_locked(ailp->xa_mount);
> + if (!XFS_FORCED_SHUTDOWN(mp))
> + xlog_assign_tail_lsn_locked(mp);
> if (list_empty(&ailp->xa_ail))
> wake_up_all(&ailp->xa_empty);
> - spin_unlock(&ailp->xa_lock);
> + }
>
> + spin_unlock(&ailp->xa_lock);
> + if (mlip_changed)
> xfs_log_space_wake(ailp->xa_mount);
> - } else {
> - spin_unlock(&ailp->xa_lock);
> - }
> }
>
> int
> diff --git a/fs/xfs/xfs_trans_priv.h b/fs/xfs/xfs_trans_priv.h
> index 49931b72da8a..d91706c56c63 100644
> --- a/fs/xfs/xfs_trans_priv.h
> +++ b/fs/xfs/xfs_trans_priv.h
> @@ -106,18 +106,9 @@ xfs_trans_ail_update(
> xfs_trans_ail_update_bulk(ailp, NULL, &lip, 1, lsn);
> }
>
> -void xfs_trans_ail_delete_bulk(struct xfs_ail *ailp,
> - struct xfs_log_item **log_items, int nr_items,
> - int shutdown_type)
> - __releases(ailp->xa_lock);
> -static inline void
> -xfs_trans_ail_delete(
> - struct xfs_ail *ailp,
> - xfs_log_item_t *lip,
> - int shutdown_type) __releases(ailp->xa_lock)
> -{
> - xfs_trans_ail_delete_bulk(ailp, &lip, 1, shutdown_type);
> -}
> +bool xfs_ail_delete_one(struct xfs_ail *ailp, struct xfs_log_item *lip);
> +void xfs_trans_ail_delete(struct xfs_ail *ailp, struct xfs_log_item *lip,
> + int shutdown_type) __releases(ailp->xa_lock);
>
> static inline void
> xfs_trans_ail_remove(
> --
> 2.11.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 5/6] xfs: don't use bool values in trace buffers
2017-04-21 19:33 ` Darrick J. Wong
@ 2017-04-23 7:38 ` Christoph Hellwig
0 siblings, 0 replies; 14+ messages in thread
From: Christoph Hellwig @ 2017-04-23 7:38 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Christoph Hellwig, linux-xfs
On Fri, Apr 21, 2017 at 12:33:21PM -0700, Darrick J. Wong wrote:
> On Fri, Apr 21, 2017 at 05:21:22PM +0200, Christoph Hellwig wrote:
> > Using bool values produces sparse warnings of this form:
> >
> > fs/xfs/./xfs_trace.h:2252:1: warning: odd constant _Bool cast (ffffffffffffffff becomes 1)
> > fs/xfs/./xfs_trace.h:2252:1: warning: odd constant _Bool cast (ffffffffffffffff becomes 1)
> > fs/xfs/./xfs_trace.h:2278:1: warning: odd constant _Bool cast (ffffffffffffffff becomes 1)
> > fs/xfs/./xfs_trace.h:2278:1: warning: odd constant _Bool cast (ffffffffffffffff becomes 1)
> > fs/xfs/./xfs_trace.h:2307:1: warning: odd constant _Bool cast (ffffffffffffffff becomes 1)
> >
> > Just use a char instead to fix those up.
> >
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> /me wonders what version of sparse produces this?
Latest git master HEAD:
commit ce18a906b82d0341cb33a71f7b1d8b98d11b345d
Author: Lance Richardson <lrichard@redhat.com>
Date: Wed Sep 21 10:13:58 2016 -0400
sparse: update __builtin_object_size() prototype
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2017-04-23 7:38 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-04-21 15:21 a few sparse inspired cleanups / fixes Christoph Hellwig
2017-04-21 15:21 ` [PATCH 1/6] xfs: use NULL instead of 0 to initialize a pointer in xfs_getfsmap Christoph Hellwig
2017-04-21 19:04 ` Darrick J. Wong
2017-04-21 15:21 ` [PATCH 2/6] xfs: use NULL instead of 0 to initialize a pointer in xfs_ioc_getfsmap Christoph Hellwig
2017-04-21 19:04 ` Darrick J. Wong
2017-04-21 15:21 ` [PATCH 3/6] xfs: corruption needs to respect endianess too! Christoph Hellwig
2017-04-21 19:05 ` Darrick J. Wong
2017-04-21 15:21 ` [PATCH 4/6] xfs: fix __user annotations for xfs_ioc_getfsmap Christoph Hellwig
2017-04-21 19:08 ` Darrick J. Wong
2017-04-21 15:21 ` [PATCH 5/6] xfs: don't use bool values in trace buffers Christoph Hellwig
2017-04-21 19:33 ` Darrick J. Wong
2017-04-23 7:38 ` Christoph Hellwig
2017-04-21 15:21 ` [PATCH 6/6] xfs: remove xfs_trans_ail_delete_bulk Christoph Hellwig
2017-04-21 19:35 ` Darrick J. Wong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox