* [REVIEW] Fix unaligned accesses in IA64 in xfsprogs
@ 2008-12-01 6:34 Barry Naujok
2008-12-01 13:42 ` Christoph Hellwig
0 siblings, 1 reply; 5+ messages in thread
From: Barry Naujok @ 2008-12-01 6:34 UTC (permalink / raw)
To: xfs@oss.sgi.com
xfs_repair is the main culprit when getting disk extents which aren't
properly aligned in memory. This patch does not call
xfs_bmbt_disk_get_all directly anymore but does an unaligned get on
the disk extent record and calls xfs_bmbt_get_all which is host-based
like the rest of the kernel routines do.
===========================================================================
xfsprogs/db/bmap.c
===========================================================================
--- a/xfsprogs/db/bmap.c 2008-12-01 17:10:37.000000000 +1100
+++ b/xfsprogs/db/bmap.c 2008-12-01 17:03:17.956547706 +1100
@@ -277,21 +277,17 @@ convert_extent(
xfs_dfilblks_t *cp,
int *fp)
{
- xfs_bmbt_irec_t irec, *s = &irec;
- xfs_bmbt_rec_t rpcopy, *p = &rpcopy;
+ xfs_bmbt_irec_t irec;
+ xfs_bmbt_rec_host_t rec;
- memmove(&rpcopy, rp, sizeof(rpcopy));
- libxfs_bmbt_disk_get_all(p, s);
-
- if (s->br_state == XFS_EXT_UNWRITTEN) {
- *fp = 1;
- } else {
- *fp = 0;
- }
-
- *op = s->br_startoff;
- *sp = s->br_startblock;
- *cp = s->br_blockcount;
+ rec.l0 = get_unaligned_be64(&rp->l0);
+ rec.l1 = get_unaligned_be64(&rp->l1);
+ libxfs_bmbt_get_all(&rec, &irec);
+
+ *fp = irec.br_state == XFS_EXT_UNWRITTEN;
+ *op = irec.br_startoff;
+ *sp = irec.br_startblock;
+ *cp = irec.br_blockcount;
}
void
===========================================================================
xfsprogs/include/libxfs.h
===========================================================================
--- a/xfsprogs/include/libxfs.h 2008-12-01 17:10:37.000000000 +1100
+++ b/xfsprogs/include/libxfs.h 2008-12-01 16:45:05.941577400 +1100
@@ -502,7 +502,7 @@ xfs_bmbt_rec_host_t *xfs_bmap_search_ext
#define libxfs_bunmapi xfs_bunmapi
/* xfs_bmap_btree.h */
-#define libxfs_bmbt_disk_get_all xfs_bmbt_disk_get_all
+#define libxfs_bmbt_get_all xfs_bmbt_get_all
/* xfs_da_btree.h */
#define libxfs_da_brelse xfs_da_brelse
===========================================================================
xfsprogs/include/xfs_arch.h
===========================================================================
--- a/xfsprogs/include/xfs_arch.h 2008-12-01 17:10:37.000000000 +1100
+++ b/xfsprogs/include/xfs_arch.h 2008-12-01 17:06:38.819726283 +1100
@@ -71,6 +71,13 @@ static inline void be64_add_cpu(__be64 *
*a = cpu_to_be64(be64_to_cpu(*a) + b);
}
+static inline __u64 get_unaligned_be64(void *ptr)
+{
+ __be64 __tmp;
+ memmove(&__tmp, ptr, 8);
+ return be64_to_cpu(__tmp);
+}
+
#endif /* __KERNEL__ */
/* do we need conversion? */
===========================================================================
xfsprogs/libxfs/xfs.h
===========================================================================
--- a/xfsprogs/libxfs/xfs.h 2008-12-01 17:10:37.000000000 +1100
+++ b/xfsprogs/libxfs/xfs.h 2008-12-01 17:05:22.041221304 +1100
@@ -127,14 +127,6 @@ static inline int __do_div(unsigned long
#define max_t(type,x,y) \
({ type __x = (x); type __y = (y); __x > __y ? __x: __y; })
-/* only 64 bit accesses used in xfs kernel code */
-static inline __u64 get_unaligned_be64(void *ptr)
-{
- __be64 __tmp;
- memmove(&__tmp, ptr, 8);
- return be64_to_cpu(__tmp);
-}
-
static inline void put_unaligned(__be64 val, void *ptr)
{
memmove(ptr, &val, 8);
===========================================================================
xfsprogs/repair/dino_chunks.c
===========================================================================
--- a/xfsprogs/repair/dino_chunks.c 2008-12-01 17:10:37.000000000 +1100
+++ b/xfsprogs/repair/dino_chunks.c 2008-12-01 16:11:11.549834281 +1100
@@ -609,7 +609,8 @@ process_inode_chunk(
if (blks_per_cluster == 0)
blks_per_cluster = 1;
cluster_count = XFS_INODES_PER_CHUNK / inodes_per_cluster;
- ASSERT(cluster_count > 0);
+ if (cluster_count == 0)
+ cluster_count = 1;
/*
* get all blocks required to read in this chunk (may wind up
===========================================================================
xfsprogs/repair/dinode.c
===========================================================================
--- a/xfsprogs/repair/dinode.c 2008-12-01 17:10:37.000000000 +1100
+++ b/xfsprogs/repair/dinode.c 2008-12-01 17:03:40.217799833 +1100
@@ -460,10 +460,13 @@ get_bmbt_reclist(
xfs_dfiloff_t fblock)
{
int i;
+ xfs_bmbt_rec_host_t rec;
xfs_bmbt_irec_t irec;
- for (i = 0; i < numrecs; i++) {
- libxfs_bmbt_disk_get_all(rp + i, &irec);
+ for (i = 0; i < numrecs; i++, rp++) {
+ rec.l0 = get_unaligned_be64(&rp->l0);
+ rec.l1 = get_unaligned_be64(&rp->l1);
+ xfs_bmbt_get_all(&rec, &irec);
if (irec.br_startoff >= fblock &&
irec.br_startoff + irec.br_blockcount < fblock)
return (irec.br_startblock + fblock - irec.br_startoff);
@@ -612,6 +615,7 @@ process_bmbt_reclist_int(
int whichfork)
{
xfs_bmbt_irec_t irec;
+ xfs_bmbt_rec_host_t rec;
xfs_dfilblks_t cp = 0; /* prev count */
xfs_dfsbno_t sp = 0; /* prev start */
xfs_dfiloff_t op = 0; /* prev offset */
@@ -636,8 +640,10 @@ process_bmbt_reclist_int(
else
ftype = _("regular");
- for (i = 0; i < numrecs; i++) {
- libxfs_bmbt_disk_get_all(rp + i, &irec);
+ for (i = 0; i < numrecs; i++, rp++) {
+ rec.l0 = get_unaligned_be64(&rp->l0);
+ rec.l1 = get_unaligned_be64(&rp->l1);
+ libxfs_bmbt_get_all(&rec, &irec);
if (i == 0)
*last_key = *first_key = irec.br_startoff;
else
@@ -913,14 +919,17 @@ getfunc_extlist(xfs_mount_t *mp,
int whichfork)
{
xfs_bmbt_irec_t irec;
+ xfs_bmbt_rec_host_t rec;
xfs_dfsbno_t final_fsbno = NULLDFSBNO;
- xfs_bmbt_rec_t *rootblock = (xfs_bmbt_rec_t *)
+ xfs_bmbt_rec_t *rp = (xfs_bmbt_rec_t *)
XFS_DFORK_PTR(dip, whichfork);
xfs_extnum_t nextents = XFS_DFORK_NEXTENTS(dip, whichfork);
int i;
- for (i = 0; i < nextents; i++) {
- libxfs_bmbt_disk_get_all(rootblock + i, &irec);
+ for (i = 0; i < nextents; i++, rp++) {
+ rec.l0 = get_unaligned_be64(&rp->l0);
+ rec.l1 = get_unaligned_be64(&rp->l1);
+ libxfs_bmbt_get_all(&rec, &irec);
if (irec.br_startoff <= bno &&
bno < irec.br_startoff + irec.br_blockcount) {
final_fsbno = bno - irec.br_startoff + irec.br_startblock;
@@ -948,6 +957,7 @@ getfunc_btree(xfs_mount_t *mp,
int found;
int numrecs;
xfs_bmbt_rec_t *rec;
+ xfs_bmbt_rec_host_t hrec;
xfs_bmbt_irec_t irec;
xfs_bmbt_ptr_t *pp;
xfs_bmbt_key_t *key;
@@ -1072,8 +1082,10 @@ getfunc_btree(xfs_mount_t *mp,
ino, numrecs, mp->m_bmap_dmnr[0]);
rec = XFS_BMBT_REC_ADDR(mp, block, 1);
- for (i = 0; i < numrecs; i++) {
- libxfs_bmbt_disk_get_all(rec + i, &irec);
+ for (i = 0; i < numrecs; i++, rec++) {
+ hrec.l0 = get_unaligned_be64(&rec->l0);
+ hrec.l1 = get_unaligned_be64(&rec->l1);
+ libxfs_bmbt_get_all(&hrec, &irec);
if (irec.br_startoff <= bno &&
bno < irec.br_startoff + irec.br_blockcount) {
final_fsbno = bno - irec.br_startoff +
@@ -1387,6 +1399,7 @@ process_symlink_extlist(xfs_mount_t *mp,
{
xfs_dfiloff_t expected_offset;
xfs_bmbt_rec_t *rp;
+ xfs_bmbt_rec_host_t rec;
xfs_bmbt_irec_t irec;
int numrecs;
int i;
@@ -1424,8 +1437,10 @@ process_symlink_extlist(xfs_mount_t *mp,
max_blocks = max_symlink_blocks;
expected_offset = 0;
- for (i = 0; i < numrecs; i++) {
- libxfs_bmbt_disk_get_all(rp + i, &irec);
+ for (i = 0; i < numrecs; i++, rp++) {
+ rec.l0 = get_unaligned_be64(&rp->l0);
+ rec.l1 = get_unaligned_be64(&rp->l1);
+ libxfs_bmbt_get_all(&rec, &irec);
if (irec.br_startoff != expected_offset) {
do_warn(
===========================================================================
xfsprogs/repair/prefetch.c
===========================================================================
--- a/xfsprogs/repair/prefetch.c 2008-12-01 17:10:37.000000000 +1100
+++ b/xfsprogs/repair/prefetch.c 2008-12-01 17:03:46.972965840 +1100
@@ -170,12 +170,15 @@ pf_read_bmbt_reclist(
int numrecs)
{
int i;
+ xfs_bmbt_rec_host_t rec;
xfs_bmbt_irec_t irec;
xfs_dfilblks_t cp = 0; /* prev count */
xfs_dfiloff_t op = 0; /* prev offset */
- for (i = 0; i < numrecs; i++) {
- libxfs_bmbt_disk_get_all(rp + i, &irec);
+ for (i = 0; i < numrecs; i++, rp++) {
+ rec.l0 = get_unaligned_be64(&rp->l0);
+ rec.l1 = get_unaligned_be64(&rp->l1);
+ libxfs_bmbt_get_all(&rec, &irec);
if (((i > 0) && (op + cp > irec.br_startoff)) ||
(irec.br_blockcount == 0) ||
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [REVIEW] Fix unaligned accesses in IA64 in xfsprogs
2008-12-01 6:34 [REVIEW] Fix unaligned accesses in IA64 in xfsprogs Barry Naujok
@ 2008-12-01 13:42 ` Christoph Hellwig
2008-12-01 23:31 ` Barry Naujok
2008-12-02 0:37 ` Barry Naujok
0 siblings, 2 replies; 5+ messages in thread
From: Christoph Hellwig @ 2008-12-01 13:42 UTC (permalink / raw)
To: Barry Naujok; +Cc: xfs@oss.sgi.com
On Mon, Dec 01, 2008 at 05:34:39PM +1100, Barry Naujok wrote:
> xfs_repair is the main culprit when getting disk extents which aren't
> properly aligned in memory. This patch does not call
> xfs_bmbt_disk_get_all directly anymore but does an unaligned get on
> the disk extent record and calls xfs_bmbt_get_all which is host-based
> like the rest of the kernel routines do.
What about just doin the get_unaligned in xfs_bmbt_disk_get_all? That
way we could just use it everywhere. The only users that don't need
the get_unaligned are in the tracing code, and I don't think we should
be worried about that little bit of overhead.
> @@ -277,21 +277,17 @@ convert_extent(
> xfs_dfilblks_t *cp,
> int *fp)
> {
And then we could replace this helper with a direct call to
xfs_bmbt_disk_get_all as the caller would be much cleaner with a
xfs_bmbt_irec_t on the stack anyway..
> --- a/xfsprogs/repair/dino_chunks.c 2008-12-01 17:10:37.000000000 +1100
> +++ b/xfsprogs/repair/dino_chunks.c 2008-12-01 16:11:11.549834281 +1100
> @@ -609,7 +609,8 @@ process_inode_chunk(
> if (blks_per_cluster == 0)
> blks_per_cluster = 1;
> cluster_count = XFS_INODES_PER_CHUNK / inodes_per_cluster;
> - ASSERT(cluster_count > 0);
> + if (cluster_count == 0)
> + cluster_count = 1;
I can't see how this is related.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [REVIEW] Fix unaligned accesses in IA64 in xfsprogs
2008-12-01 13:42 ` Christoph Hellwig
@ 2008-12-01 23:31 ` Barry Naujok
2008-12-02 0:37 ` Barry Naujok
1 sibling, 0 replies; 5+ messages in thread
From: Barry Naujok @ 2008-12-01 23:31 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs@oss.sgi.com
On Tue, 02 Dec 2008 00:42:05 +1100, Christoph Hellwig <hch@infradead.org>
wrote:
> On Mon, Dec 01, 2008 at 05:34:39PM +1100, Barry Naujok wrote:
>> xfs_repair is the main culprit when getting disk extents which aren't
>> properly aligned in memory. This patch does not call
>> xfs_bmbt_disk_get_all directly anymore but does an unaligned get on
>> the disk extent record and calls xfs_bmbt_get_all which is host-based
>> like the rest of the kernel routines do.
>
> What about just doin the get_unaligned in xfs_bmbt_disk_get_all? That
> way we could just use it everywhere. The only users that don't need
> the get_unaligned are in the tracing code, and I don't think we should
> be worried about that little bit of overhead.
>
>> @@ -277,21 +277,17 @@ convert_extent(
>> xfs_dfilblks_t *cp,
>> int *fp)
>> {
>
> And then we could replace this helper with a direct call to
> xfs_bmbt_disk_get_all as the caller would be much cleaner with a
> xfs_bmbt_irec_t on the stack anyway..
It's a libxfs/kernel function, so ideally, it should be also ported
into the kernel space and possible kernel cleanups along with it.
>> --- a/xfsprogs/repair/dino_chunks.c 2008-12-01 17:10:37.000000000 +1100
>> +++ b/xfsprogs/repair/dino_chunks.c 2008-12-01 16:11:11.549834281 +1100
>> @@ -609,7 +609,8 @@ process_inode_chunk(
>> if (blks_per_cluster == 0)
>> blks_per_cluster = 1;
>> cluster_count = XFS_INODES_PER_CHUNK / inodes_per_cluster;
>> - ASSERT(cluster_count > 0);
>> + if (cluster_count == 0)
>> + cluster_count = 1;
>
> I can't see how this is related.
Another IA64 fix.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [REVIEW] Fix unaligned accesses in IA64 in xfsprogs
2008-12-01 13:42 ` Christoph Hellwig
2008-12-01 23:31 ` Barry Naujok
@ 2008-12-02 0:37 ` Barry Naujok
2008-12-02 6:46 ` Christoph Hellwig
1 sibling, 1 reply; 5+ messages in thread
From: Barry Naujok @ 2008-12-02 0:37 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs@oss.sgi.com
On Tue, 02 Dec 2008 00:42:05 +1100, Christoph Hellwig <hch@infradead.org>
wrote:
> On Mon, Dec 01, 2008 at 05:34:39PM +1100, Barry Naujok wrote:
>> xfs_repair is the main culprit when getting disk extents which aren't
>> properly aligned in memory. This patch does not call
>> xfs_bmbt_disk_get_all directly anymore but does an unaligned get on
>> the disk extent record and calls xfs_bmbt_get_all which is host-based
>> like the rest of the kernel routines do.
>
> What about just doin the get_unaligned in xfs_bmbt_disk_get_all? That
> way we could just use it everywhere. The only users that don't need
> the get_unaligned are in the tracing code, and I don't think we should
> be worried about that little bit of overhead.
>
>> @@ -277,21 +277,17 @@ convert_extent(
>> xfs_dfilblks_t *cp,
>> int *fp)
>> {
>
> And then we could replace this helper with a direct call to
> xfs_bmbt_disk_get_all as the caller would be much cleaner with a
> xfs_bmbt_irec_t on the stack anyway..
Obviously modifying xfs_bmbt_disk_get_all yields a much smaller patch:
===========================================================================
xfsprogs/db/bmap.c
===========================================================================
--- a/xfsprogs/db/bmap.c 2008-12-02 11:21:00.000000000 +1100
+++ b/xfsprogs/db/bmap.c 2008-12-02 11:20:41.324928232 +1100
@@ -277,21 +277,14 @@ convert_extent(
xfs_dfilblks_t *cp,
int *fp)
{
- xfs_bmbt_irec_t irec, *s = &irec;
- xfs_bmbt_rec_t rpcopy, *p = &rpcopy;
+ xfs_bmbt_irec_t irec;
- memmove(&rpcopy, rp, sizeof(rpcopy));
- libxfs_bmbt_disk_get_all(p, s);
+ libxfs_bmbt_disk_get_all(rp, &irec);
- if (s->br_state == XFS_EXT_UNWRITTEN) {
- *fp = 1;
- } else {
- *fp = 0;
- }
-
- *op = s->br_startoff;
- *sp = s->br_startblock;
- *cp = s->br_blockcount;
+ *fp = irec.br_state == XFS_EXT_UNWRITTEN;
+ *op = irec.br_startoff;
+ *sp = irec.br_startblock;
+ *cp = irec.br_blockcount;
}
void
===========================================================================
xfsprogs/libxfs/xfs_bmap_btree.c
===========================================================================
--- a/xfsprogs/libxfs/xfs_bmap_btree.c 2008-12-02 11:21:00.000000000 +1100
+++ b/xfsprogs/libxfs/xfs_bmap_btree.c 2008-12-02 11:20:09.553355392 +1100
@@ -181,7 +181,8 @@ xfs_bmbt_disk_get_all(
xfs_bmbt_rec_t *r,
xfs_bmbt_irec_t *s)
{
- __xfs_bmbt_get_all(be64_to_cpu(r->l0), be64_to_cpu(r->l1), s);
+ __xfs_bmbt_get_all(get_unaligned_be64(&r->l0),
+ get_unaligned_be64(&r->l1), s);
}
/*
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [REVIEW] Fix unaligned accesses in IA64 in xfsprogs
2008-12-02 0:37 ` Barry Naujok
@ 2008-12-02 6:46 ` Christoph Hellwig
0 siblings, 0 replies; 5+ messages in thread
From: Christoph Hellwig @ 2008-12-02 6:46 UTC (permalink / raw)
To: Barry Naujok; +Cc: Christoph Hellwig, xfs@oss.sgi.com
This patch looks good to go to me.
On Tue, Dec 02, 2008 at 11:37:31AM +1100, Barry Naujok wrote:
> Obviously modifying xfs_bmbt_disk_get_all yields a much smaller patch:
>
> ===========================================================================
> xfsprogs/db/bmap.c
> ===========================================================================
>
> --- a/xfsprogs/db/bmap.c 2008-12-02 11:21:00.000000000 +1100
> +++ b/xfsprogs/db/bmap.c 2008-12-02 11:20:41.324928232 +1100
> @@ -277,21 +277,14 @@ convert_extent(
> xfs_dfilblks_t *cp,
> int *fp)
> {
> - xfs_bmbt_irec_t irec, *s = &irec;
> - xfs_bmbt_rec_t rpcopy, *p = &rpcopy;
> + xfs_bmbt_irec_t irec;
>
> - memmove(&rpcopy, rp, sizeof(rpcopy));
> - libxfs_bmbt_disk_get_all(p, s);
> + libxfs_bmbt_disk_get_all(rp, &irec);
>
> - if (s->br_state == XFS_EXT_UNWRITTEN) {
> - *fp = 1;
> - } else {
> - *fp = 0;
> - }
> -
> - *op = s->br_startoff;
> - *sp = s->br_startblock;
> - *cp = s->br_blockcount;
> + *fp = irec.br_state == XFS_EXT_UNWRITTEN;
> + *op = irec.br_startoff;
> + *sp = irec.br_startblock;
> + *cp = irec.br_blockcount;
> }
>
> void
>
> ===========================================================================
> xfsprogs/libxfs/xfs_bmap_btree.c
> ===========================================================================
>
> --- a/xfsprogs/libxfs/xfs_bmap_btree.c 2008-12-02 11:21:00.000000000 +1100
> +++ b/xfsprogs/libxfs/xfs_bmap_btree.c 2008-12-02 11:20:09.553355392 +1100
> @@ -181,7 +181,8 @@ xfs_bmbt_disk_get_all(
> xfs_bmbt_rec_t *r,
> xfs_bmbt_irec_t *s)
> {
> - __xfs_bmbt_get_all(be64_to_cpu(r->l0), be64_to_cpu(r->l1), s);
> + __xfs_bmbt_get_all(get_unaligned_be64(&r->l0),
> + get_unaligned_be64(&r->l1), s);
> }
>
> /*
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
---end quoted text---
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2008-12-02 6:52 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-12-01 6:34 [REVIEW] Fix unaligned accesses in IA64 in xfsprogs Barry Naujok
2008-12-01 13:42 ` Christoph Hellwig
2008-12-01 23:31 ` Barry Naujok
2008-12-02 0:37 ` Barry Naujok
2008-12-02 6:46 ` Christoph Hellwig
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox