2006-01-13 00:37:44

by Alexey Dobriyan

[permalink] [raw]
Subject: Oops in ufs_fill_super at mount time

Version 2.6.15-43ecb9a33ba8c93ebbda81d48ca05f0d1bbf9056

Actually more or less latest -git is affected too, but
I'm sick of recompiling right now so please wait for bisecting results.

It's random wrt one mount of UFS, but several mount/umounts in a row
reproduce it reliably:

mount -t ufs -o ro,ufstype=44bsd /dev/hda9 /home/openbsd/usr/

I've tracked this down to line 926 of fs/ufs/super.c:

921 uspi->s_ntrak = fs32_to_cpu(sb, usb1->fs_ntrak);
922 uspi->s_nsect = fs32_to_cpu(sb, usb1->fs_nsect);
923 uspi->s_spc = fs32_to_cpu(sb, usb1->fs_spc);
924 uspi->s_ipg = fs32_to_cpu(sb, usb1->fs_ipg);
925 uspi->s_fpg = fs32_to_cpu(sb, usb1->fs_fpg);
926 ===> uspi->s_cpc = fs32_to_cpu(sb, usb2->fs_cpc); <=== Oops here
927 uspi->s_contigsumsize = fs32_to_cpu(sb, usb3->fs_u2.fs_44.fs_contigsumsize);
928 uspi->s_qbmask = ufs_get_fs_qbmask(sb, usb3);
929 uspi->s_qfmask = ufs_get_fs_qfmask(sb, usb3);
930 uspi->s_postblformat = fs32_to_cpu(sb, usb3->fs_postblformat);

(fs/ufs/super.c, 1029), ufs_put_super: ENTER
(fs/ufs/super.c, 545), ufs_fill_super: ENTER
(fs/ufs/super.c, 553), ufs_fill_super: flag 1
(fs/ufs/super.c, 310), ufs_parse_options: ENTER
(fs/ufs/super.c, 593), ufs_fill_super: ufstype=44bsd
(fs/ufs/super.c, 822), ufs_fill_super: another value of block_size or super_block_size 2048, 2048
ufs_print_super_stuff
size of usb: 1380
magic: 0x11954
sblkno: 8
cblkno: 16
iblkno: 24
dblkno: 1312
cgoffset: 16
~cgmask: 0xf
size: 2097144
dsize: 2063231
ncg: 26
bsize: 16384
fsize: 2048
frag: 8
fragshift: 3
~fmask: 2047
fshift: 11
sbsize: 2048
spc: 1008
cpg: 328
ipg: 20608
fpg: 82656
csaddr: 1312
cssize: 2048
cgsize: 16384
fstodb: 2
contigsumsize: 3
postblformat: 1
nrpos: 1
ndir 29518
nifree 339567
nbfree 107711
nffree 5167

(fs/ufs/super.c, 844), ufs_fill_super: fs is clean
(fs/ufs/super.c, 904), ufs_fill_super: uspi->s_bshift = 14,uspi->s_fshift = 11
before uspi->s_cpc = fs32_to_cpu(sb, usb2->fs_cpc);
Unable to handle kernel paging request at virtual address d734c158
printing eip:
c019f138
*pde = 0005c067
*pte = 1734c000
Oops: 0000 [#1]
PREEMPT DEBUG_PAGEALLOC
Modules linked in: snd_pcm_oss snd_mixer_oss snd_intel8x0 snd_ac97_codec snd_ac97_bus snd_pcm snd_timer snd soundcore snd_page_alloc ehci_hcd uhci_hcd usbcore
CPU: 0
EIP: 0060:[<c019f138>] Not tainted VLI
EFLAGS: 00010286 (2.6.15-43ecb9a33ba8c93ebbda81d48ca05f0d1bbf9056)
EIP is at ufs_fill_super+0x1094/0x151e
eax: d734c000 ebx: 00000000 ecx: dd270e4c edx: c0289c4b
esi: dd378df8 edi: 00000800 ebp: dd394df8 esp: dd270e4c
ds: 007b es: 007b ss: 0068
Process mount (pid: 7857, threadinfo=dd270000 task=ddb70b00)
Stack: <0>dd270ec3 00000020 d7b7c374 c0283146 dd270e88 dd270ea4 dd325ef8 00002130
dd378df8 d730c400 d734c000 d730c000 dd378df8 00000020 c0283142 00001000
dd394df8 dd394df8 d730be5c 00000000 d7309000 c014a1ce 39616468 c0142000
Call Trace:
[<c014a1ce>] get_sb_bdev+0xbf/0xfa
[<c0142000>] kmem_cache_alloc+0x59/0x5c
[<c015afb3>] alloc_vfsmnt+0x97/0xbe
[<c015afb3>] alloc_vfsmnt+0x97/0xbe
[<c019fc31>] ufs_get_sb+0xe/0x11
[<c019e0a4>] ufs_fill_super+0x0/0x151e
[<c014a340>] do_kern_mount+0x3b/0x9d
[<c015c22a>] do_new_mount+0x61/0x90
[<c015c7d4>] do_mount+0x161/0x179
[<c012e8d3>] __alloc_pages+0x47/0x256
[<c015cab5>] sys_mount+0x6f/0xad
[<c01025fd>] syscall_call+0x7/0xb
Code: 91 bc 00 00 00 83 b8 80 00 00 00 00 89 d1 74 02 0f c9 8b 74 24 30 89 8e cc 00 00 00 68 4b 9c 28 c0 e8 ec 25 f7 ff 58 8b 44 24 28 <8b> 90 58 01 00 00 8b 85 68 01 00 00 83 b8 80 00 00 00 00 89 d1


2006-01-13 01:14:31

by Linus Torvalds

[permalink] [raw]
Subject: Re: Oops in ufs_fill_super at mount time



On Fri, 13 Jan 2006, Alexey Dobriyan wrote:
>
> Version 2.6.15-43ecb9a33ba8c93ebbda81d48ca05f0d1bbf9056
>
> Actually more or less latest -git is affected too, but
> I'm sick of recompiling right now so please wait for bisecting results.

Hmm.. I don't see any recent changes that could affect this. Not after
2.6.15, but in fact not even after 2.6.14.

Your oops is also interesting in another way...

> Unable to handle kernel paging request at virtual address d734c158
> printing eip:
> c019f138
> *pde = 0005c067
> *pte = 1734c000
> Oops: 0000 [#1]
> PREEMPT DEBUG_PAGEALLOC

This is a free'd page fault, so it's due to DEBUG_PAGEALLOC rather than a
wild pointer.

Is that something new for you? Maybe the bug is older, but you've enabled
PAGEALLOC only recently? Also, out of interest, have you enabled slab
debugging?

That said, the whole ubh_get_usb_second() and ubh_get_usb_third() thing is
pretty damn scary. There's no testing of the values passed in at all and
comparing them to the allocated buffer heads. But from what I can tell,
ubh_bread_uspi() will zero out any unallocated bh's, and it certainly
_looks_ like the calculations to calculate "usb2" should fit within the
sectors that were read..

Very strange.

Linus

2006-01-13 10:11:07

by Alexey Dobriyan

[permalink] [raw]
Subject: Re: Oops in ufs_fill_super at mount time

On Thu, Jan 12, 2006 at 05:14:25PM -0800, Linus Torvalds wrote:
> On Fri, 13 Jan 2006, Alexey Dobriyan wrote:
> >
> > Version 2.6.15-43ecb9a33ba8c93ebbda81d48ca05f0d1bbf9056
> >
> > Actually more or less latest -git is affected too, but
> > I'm sick of recompiling right now so please wait for bisecting results.
>
> Hmm.. I don't see any recent changes that could affect this. Not after
> 2.6.15, but in fact not even after 2.6.14.
>
> Your oops is also interesting in another way...
>
> > Unable to handle kernel paging request at virtual address d734c158
> > printing eip:
> > c019f138
> > *pde = 0005c067
> > *pte = 1734c000
> > Oops: 0000 [#1]
> > PREEMPT DEBUG_PAGEALLOC
>
> This is a free'd page fault, so it's due to DEBUG_PAGEALLOC rather than a
> wild pointer.

That's true. Turning it off makes mounting reliable again.

> Is that something new for you? Maybe the bug is older, but you've enabled
> PAGEALLOC only recently?

Yup. In response to hangs re disk activity.

> Also, out of interest, have you enabled slab
> debugging?

Yup.

> That said, the whole ubh_get_usb_second() and ubh_get_usb_third() thing is
> pretty damn scary. There's no testing of the values passed in at all and
> comparing them to the allocated buffer heads. But from what I can tell,
> ubh_bread_uspi() will zero out any unallocated bh's, and it certainly
> _looks_ like the calculations to calculate "usb2" should fit within the
> sectors that were read..
>
> Very strange.

2006-01-13 15:26:13

by Evgeniy Dushistov

[permalink] [raw]
Subject: [PATCH 2.6.15] Re: Oops in ufs_fill_super at mount time

On Fri, Jan 13, 2006 at 03:54:50AM +0300, Alexey Dobriyan wrote:
> Version 2.6.15-43ecb9a33ba8c93ebbda81d48ca05f0d1bbf9056
>
> Actually more or less latest -git is affected too, but
> I'm sick of recompiling right now so please wait for bisecting results.
>
> It's random wrt one mount of UFS, but several mount/umounts in a row
> reproduce it reliably:
>

I suppose problem in a couple of brackets in fs/ufs/utils.h,
instead of 512th byte of buffer, usb2 point to nth structure of
type ufs_super_block_second.

This patch fix problem for me,
can you test it?

Signed-off-by: Evgeniy Dushistov <[email protected]>

---

--- linux-2.6.15/fs/ufs/util.h.orig 2006-01-13 17:33:49.123946250 +0300
+++ linux-2.6.15/fs/ufs/util.h 2006-01-13 17:35:50.127508500 +0300
@@ -255,8 +255,8 @@ extern void _ubh_memcpyubh_(struct ufs_s
((struct ufs_super_block_first *)((ubh)->bh[0]->b_data))

#define ubh_get_usb_second(ubh) \
- ((struct ufs_super_block_second *)(ubh)-> \
- bh[UFS_SECTOR_SIZE >> uspi->s_fshift]->b_data + (UFS_SECTOR_SIZE & ~uspi->s_fmask))
+ ((struct ufs_super_block_second *)((ubh)->\
+ bh[UFS_SECTOR_SIZE >> uspi->s_fshift]->b_data + (UFS_SECTOR_SIZE & ~uspi->s_fmask)))

#define ubh_get_usb_third(ubh) \
((struct ufs_super_block_third *)((ubh)-> \

--
/Evgeniy

2006-01-13 15:45:24

by Linus Torvalds

[permalink] [raw]
Subject: Re: Oops in ufs_fill_super at mount time



On Fri, 13 Jan 2006, Alexey Dobriyan wrote:

> On Thu, Jan 12, 2006 at 05:14:25PM -0800, Linus Torvalds wrote:
> >
> > This is a free'd page fault, so it's due to DEBUG_PAGEALLOC rather than a
> > wild pointer.
>
> That's true. Turning it off makes mounting reliable again.
>
> > Is that something new for you? Maybe the bug is older, but you've enabled
> > PAGEALLOC only recently?
>
> Yup. In response to hangs re disk activity.

Ok, That explains why it started happening for you only _now_, but not why
it happens in the first place.

Can you test if the patch that Evgeniy sent out fixes it for you even with
PAGEALLOC debugging enabled?

Evgeniy - That is one ugly macro, can you (or Alexey, for that matter:
somebody who can test it) turn it into an inline function or something to
make it half-way readable? I realize that means changing the arguments too
(right now that horrid macro accesses "uspi" directly - uggghhh).

If somebody maintains - or is interested in doing so - UFS, please speak
up, we don't have anybody listed in the MAINTAINERS file, and when I look
through the history, all I see is updates due to secondary issues (ie
somebody did generic cleanups and just happened to touch UFS as part of
that, rather than working _directly_ on UFS issues).

Linus

2006-01-13 16:18:50

by Alexey Dobriyan

[permalink] [raw]
Subject: Re: Oops in ufs_fill_super at mount time

On Fri, Jan 13, 2006 at 07:45:12AM -0800, Linus Torvalds wrote:
> On Fri, 13 Jan 2006, Alexey Dobriyan wrote:
>
> > On Thu, Jan 12, 2006 at 05:14:25PM -0800, Linus Torvalds wrote:
> > >
> > > This is a free'd page fault, so it's due to DEBUG_PAGEALLOC rather than a
> > > wild pointer.
> >
> > That's true. Turning it off makes mounting reliable again.
> >
> > > Is that something new for you? Maybe the bug is older, but you've enabled
> > > PAGEALLOC only recently?
> >
> > Yup. In response to hangs re disk activity.
>
> Ok, That explains why it started happening for you only _now_, but not why
> it happens in the first place.
>
> Can you test if the patch that Evgeniy sent out fixes it for you even with
> PAGEALLOC debugging enabled?

It does the trick! And you saved me from going before 2.6.0. ;-)

2006-01-13 19:18:27

by Evgeniy Dushistov

[permalink] [raw]
Subject: [PATCH 2.6.15] ufs cleanup

On Fri, Jan 13, 2006 at 07:45:12AM -0800, Linus Torvalds wrote:
> Evgeniy - That is one ugly macro, can you (or Alexey, for that matter:
> somebody who can test it) turn it into an inline function or something to
> make it half-way readable? I realize that means changing the arguments too
> (right now that horrid macro accesses "uspi" directly - uggghhh).

You mean something like this?

This patch converts ubh_get_usb_* to inline functions.

Signed-off-by: Evgeniy Dushistov <[email protected]>

---


diff -uprN -X linux-2.6.15-vanilla/Documentation/dontdiff linux-2.6.15-vanilla/fs/ufs/balloc.c linux-2.6.15/fs/ufs/balloc.c
--- linux-2.6.15-vanilla/fs/ufs/balloc.c 2006-01-03 06:21:10.000000000 +0300
+++ linux-2.6.15/fs/ufs/balloc.c 2006-01-13 21:32:50.308214250 +0300
@@ -48,7 +48,7 @@ void ufs_free_fragments (struct inode *

sb = inode->i_sb;
uspi = UFS_SB(sb)->s_uspi;
- usb1 = ubh_get_usb_first(USPI_UBH);
+ usb1 = ubh_get_usb_first(uspi);

UFSD(("ENTER, fragment %u, count %u\n", fragment, count))

@@ -142,7 +142,7 @@ void ufs_free_blocks (struct inode * ino

sb = inode->i_sb;
uspi = UFS_SB(sb)->s_uspi;
- usb1 = ubh_get_usb_first(USPI_UBH);
+ usb1 = ubh_get_usb_first(uspi);

UFSD(("ENTER, fragment %u, count %u\n", fragment, count))

@@ -246,7 +246,7 @@ unsigned ufs_new_fragments (struct inode

sb = inode->i_sb;
uspi = UFS_SB(sb)->s_uspi;
- usb1 = ubh_get_usb_first(USPI_UBH);
+ usb1 = ubh_get_usb_first(uspi);
*err = -ENOSPC;

lock_super (sb);
@@ -406,7 +406,7 @@ ufs_add_fragments (struct inode * inode,

sb = inode->i_sb;
uspi = UFS_SB(sb)->s_uspi;
- usb1 = ubh_get_usb_first (USPI_UBH);
+ usb1 = ubh_get_usb_first (uspi);
count = newcount - oldcount;

cgno = ufs_dtog(fragment);
@@ -489,7 +489,7 @@ static unsigned ufs_alloc_fragments (str

sb = inode->i_sb;
uspi = UFS_SB(sb)->s_uspi;
- usb1 = ubh_get_usb_first(USPI_UBH);
+ usb1 = ubh_get_usb_first(uspi);
oldcg = cgno;

/*
@@ -605,7 +605,7 @@ static unsigned ufs_alloccg_block (struc

sb = inode->i_sb;
uspi = UFS_SB(sb)->s_uspi;
- usb1 = ubh_get_usb_first(USPI_UBH);
+ usb1 = ubh_get_usb_first(uspi);
ucg = ubh_get_ucg(UCPI_UBH);

if (goal == 0) {
@@ -662,7 +662,7 @@ static unsigned ufs_bitmap_search (struc
UFSD(("ENTER, cg %u, goal %u, count %u\n", ucpi->c_cgx, goal, count))

uspi = UFS_SB(sb)->s_uspi;
- usb1 = ubh_get_usb_first (USPI_UBH);
+ usb1 = ubh_get_usb_first (uspi);
ucg = ubh_get_ucg(UCPI_UBH);

if (goal)
diff -uprN -X linux-2.6.15-vanilla/Documentation/dontdiff linux-2.6.15-vanilla/fs/ufs/ialloc.c linux-2.6.15/fs/ufs/ialloc.c
--- linux-2.6.15-vanilla/fs/ufs/ialloc.c 2006-01-03 06:21:10.000000000 +0300
+++ linux-2.6.15/fs/ufs/ialloc.c 2006-01-13 21:32:50.328215500 +0300
@@ -72,7 +72,7 @@ void ufs_free_inode (struct inode * inod

sb = inode->i_sb;
uspi = UFS_SB(sb)->s_uspi;
- usb1 = ubh_get_usb_first(USPI_UBH);
+ usb1 = ubh_get_usb_first(uspi);

ino = inode->i_ino;

@@ -167,7 +167,7 @@ struct inode * ufs_new_inode(struct inod
ufsi = UFS_I(inode);
sbi = UFS_SB(sb);
uspi = sbi->s_uspi;
- usb1 = ubh_get_usb_first(USPI_UBH);
+ usb1 = ubh_get_usb_first(uspi);

lock_super (sb);

diff -uprN -X linux-2.6.15-vanilla/Documentation/dontdiff linux-2.6.15-vanilla/fs/ufs/super.c linux-2.6.15/fs/ufs/super.c
--- linux-2.6.15-vanilla/fs/ufs/super.c 2006-01-03 06:21:10.000000000 +0300
+++ linux-2.6.15/fs/ufs/super.c 2006-01-13 21:32:50.336216000 +0300
@@ -221,7 +221,7 @@ void ufs_error (struct super_block * sb,
va_list args;

uspi = UFS_SB(sb)->s_uspi;
- usb1 = ubh_get_usb_first(USPI_UBH);
+ usb1 = ubh_get_usb_first(uspi);

if (!(sb->s_flags & MS_RDONLY)) {
usb1->fs_clean = UFS_FSBAD;
@@ -253,7 +253,7 @@ void ufs_panic (struct super_block * sb,
va_list args;

uspi = UFS_SB(sb)->s_uspi;
- usb1 = ubh_get_usb_first(USPI_UBH);
+ usb1 = ubh_get_usb_first(uspi);

if (!(sb->s_flags & MS_RDONLY)) {
usb1->fs_clean = UFS_FSBAD;
@@ -735,9 +735,9 @@ again:
goto failed;


- usb1 = ubh_get_usb_first(USPI_UBH);
- usb2 = ubh_get_usb_second(USPI_UBH);
- usb3 = ubh_get_usb_third(USPI_UBH);
+ usb1 = ubh_get_usb_first(uspi);
+ usb2 = ubh_get_usb_second(uspi);
+ usb3 = ubh_get_usb_third(uspi);
usb = (struct ufs_super_block *)
((struct ufs_buffer_head *)uspi)->bh[0]->b_data ;

@@ -1006,8 +1006,8 @@ static void ufs_write_super (struct supe
UFSD(("ENTER\n"))
flags = UFS_SB(sb)->s_flags;
uspi = UFS_SB(sb)->s_uspi;
- usb1 = ubh_get_usb_first(USPI_UBH);
- usb3 = ubh_get_usb_third(USPI_UBH);
+ usb1 = ubh_get_usb_first(uspi);
+ usb3 = ubh_get_usb_third(uspi);

if (!(sb->s_flags & MS_RDONLY)) {
usb1->fs_time = cpu_to_fs32(sb, get_seconds());
@@ -1049,8 +1049,8 @@ static int ufs_remount (struct super_blo

uspi = UFS_SB(sb)->s_uspi;
flags = UFS_SB(sb)->s_flags;
- usb1 = ubh_get_usb_first(USPI_UBH);
- usb3 = ubh_get_usb_third(USPI_UBH);
+ usb1 = ubh_get_usb_first(uspi);
+ usb3 = ubh_get_usb_third(uspi);

/*
* Allow the "check" option to be passed as a remount option.
@@ -1124,7 +1124,7 @@ static int ufs_statfs (struct super_bloc
lock_kernel();

uspi = UFS_SB(sb)->s_uspi;
- usb1 = ubh_get_usb_first (USPI_UBH);
+ usb1 = ubh_get_usb_first (uspi);
usb = (struct ufs_super_block *)
((struct ufs_buffer_head *)uspi)->bh[0]->b_data ;

diff -uprN -X linux-2.6.15-vanilla/Documentation/dontdiff linux-2.6.15-vanilla/fs/ufs/util.h linux-2.6.15/fs/ufs/util.h
--- linux-2.6.15-vanilla/fs/ufs/util.h 2006-01-13 21:28:46.724991250 +0300
+++ linux-2.6.15/fs/ufs/util.h 2006-01-13 21:32:50.344216500 +0300
@@ -249,18 +249,30 @@ extern void _ubh_memcpyubh_(struct ufs_s


/*
- * macros to get important structures from ufs_buffer_head
+ * inline functions to get important structures from ufs_sb_private_info
*/
-#define ubh_get_usb_first(ubh) \
- ((struct ufs_super_block_first *)((ubh)->bh[0]->b_data))
+static inline struct ufs_super_block_first *
+ubh_get_usb_first(struct ufs_sb_private_info *uspi)
+{
+ char *res=uspi->s_ubh.bh[0]->b_data;
+ return (struct ufs_super_block_first *)res;
+}

-#define ubh_get_usb_second(ubh) \
- ((struct ufs_super_block_second *)((ubh)->\
- bh[UFS_SECTOR_SIZE >> uspi->s_fshift]->b_data + (UFS_SECTOR_SIZE & ~uspi->s_fmask)))
-
-#define ubh_get_usb_third(ubh) \
- ((struct ufs_super_block_third *)((ubh)-> \
- bh[UFS_SECTOR_SIZE*2 >> uspi->s_fshift]->b_data + (UFS_SECTOR_SIZE*2 & ~uspi->s_fmask)))
+static inline struct ufs_super_block_second *
+ubh_get_usb_second(struct ufs_sb_private_info *uspi)
+{
+ char *res=uspi->s_ubh.bh[UFS_SECTOR_SIZE >> uspi->s_fshift]->b_data +
+ (UFS_SECTOR_SIZE & ~uspi->s_fmask);
+ return (struct ufs_super_block_second *)res;
+}
+
+static inline struct ufs_super_block_third *
+ubh_get_usb_third(struct ufs_sb_private_info *uspi)
+{
+ char *res=uspi->s_ubh.bh[UFS_SECTOR_SIZE*2 >> uspi->s_fshift]->b_data +
+ (UFS_SECTOR_SIZE*2 & ~uspi->s_fmask);
+ return (struct ufs_super_block_third *)res;
+}

#define ubh_get_ucg(ubh) \
((struct ufs_cylinder_group *)((ubh)->bh[0]->b_data))

--
/Evgeniy

2006-01-13 19:54:34

by Linus Torvalds

[permalink] [raw]
Subject: Re: [PATCH 2.6.15] ufs cleanup



On Fri, 13 Jan 2006, Evgeniy wrote:
> +static inline struct ufs_super_block_second *
> +ubh_get_usb_second(struct ufs_sb_private_info *uspi)
> +{
> + char *res=uspi->s_ubh.bh[UFS_SECTOR_SIZE >> uspi->s_fshift]->b_data +
> + (UFS_SECTOR_SIZE & ~uspi->s_fmask);
> + return (struct ufs_super_block_second *)res;
> +}

I was thinking of something even more abstracted:

static inline void *get_usb_offset(struct ufs_sb_private_info *uspi,
unsigned int offset)
{
unsigned int index;

index = offset >> uspi->s_fshift;
offset &= ~uspi->s_fmask;
return uspi->s_ubh.bh[index]->b_data + offset;
}

and then just doing

#define ubs_get_usb_first(uspi) \
((struct ufs_super_block_first *)get_usb_offset(uspi, 0))

#define ubh_get_usb_second(uspi) \
((struct ufs_super_block_second *)get_usb_offset(uspi, UFS_SECTOR_SIZE))

#define ubh_get_usb_third(uspi) \
((struct ufs_super_block_third *)get_usb_offset(uspi, 2*UFS_SECTOR_SIZE))

or something similar. Which seems a hell of a lot more readable to me, and
assuming it passes testing (ie I didn't screw up), I think it's more
likely to stay correct in the future and just generally be maintainable.

Hmm?

Linus

2006-01-14 08:55:09

by Evgeniy Dushistov

[permalink] [raw]
Subject: Re: [PATCH 2.6.15] ufs cleanup v. 2

On Fri, Jan 13, 2006 at 11:51:28AM -0800, Linus Torvalds wrote:
> I was thinking of something even more abstracted:
>
> static inline void *get_usb_offset(struct ufs_sb_private_info *uspi,
> unsigned int offset)
> {
> unsigned int index;
>
> index = offset >> uspi->s_fshift;
> offset &= ~uspi->s_fmask;
> return uspi->s_ubh.bh[index]->b_data + offset;
> }
> Hmm?

Yes, this is much better then my suggestion.

Here is update of ufs cleanup patch,
it also includes
* fix compilation warnings which appears if debug mode turn on
* remove unnecessary duplication of code to support UFS2

I tested it on ufs1 and ufs2 file-systems.

Signed-off-by: Evgeniy Dushistov <[email protected]>

---

diff -uprN -X linux-2.6.15-vanilla/Documentation/dontdiff linux-2.6.15-vanilla/fs/ufs/balloc.c linux-2.6.15/fs/ufs/balloc.c
--- linux-2.6.15-vanilla/fs/ufs/balloc.c 2006-01-03 06:21:10.000000000 +0300
+++ linux-2.6.15/fs/ufs/balloc.c 2006-01-14 09:50:06.768125250 +0300
@@ -48,7 +48,7 @@ void ufs_free_fragments (struct inode *

sb = inode->i_sb;
uspi = UFS_SB(sb)->s_uspi;
- usb1 = ubh_get_usb_first(USPI_UBH);
+ usb1 = ubh_get_usb_first(uspi);

UFSD(("ENTER, fragment %u, count %u\n", fragment, count))

@@ -80,8 +80,9 @@ void ufs_free_fragments (struct inode *
for (i = bit; i < end_bit; i++) {
if (ubh_isclr (UCPI_UBH, ucpi->c_freeoff, i))
ubh_setbit (UCPI_UBH, ucpi->c_freeoff, i);
- else ufs_error (sb, "ufs_free_fragments",
- "bit already cleared for fragment %u", i);
+ else
+ ufs_error (sb, "ufs_free_fragments",
+ "bit already cleared for fragment %u", i);
}

DQUOT_FREE_BLOCK (inode, count);
@@ -142,7 +143,7 @@ void ufs_free_blocks (struct inode * ino

sb = inode->i_sb;
uspi = UFS_SB(sb)->s_uspi;
- usb1 = ubh_get_usb_first(USPI_UBH);
+ usb1 = ubh_get_usb_first(uspi);

UFSD(("ENTER, fragment %u, count %u\n", fragment, count))

@@ -246,7 +247,7 @@ unsigned ufs_new_fragments (struct inode

sb = inode->i_sb;
uspi = UFS_SB(sb)->s_uspi;
- usb1 = ubh_get_usb_first(USPI_UBH);
+ usb1 = ubh_get_usb_first(uspi);
*err = -ENOSPC;

lock_super (sb);
@@ -406,7 +407,7 @@ ufs_add_fragments (struct inode * inode,

sb = inode->i_sb;
uspi = UFS_SB(sb)->s_uspi;
- usb1 = ubh_get_usb_first (USPI_UBH);
+ usb1 = ubh_get_usb_first (uspi);
count = newcount - oldcount;

cgno = ufs_dtog(fragment);
@@ -489,7 +490,7 @@ static unsigned ufs_alloc_fragments (str

sb = inode->i_sb;
uspi = UFS_SB(sb)->s_uspi;
- usb1 = ubh_get_usb_first(USPI_UBH);
+ usb1 = ubh_get_usb_first(uspi);
oldcg = cgno;

/*
@@ -605,7 +606,7 @@ static unsigned ufs_alloccg_block (struc

sb = inode->i_sb;
uspi = UFS_SB(sb)->s_uspi;
- usb1 = ubh_get_usb_first(USPI_UBH);
+ usb1 = ubh_get_usb_first(uspi);
ucg = ubh_get_ucg(UCPI_UBH);

if (goal == 0) {
@@ -662,7 +663,7 @@ static unsigned ufs_bitmap_search (struc
UFSD(("ENTER, cg %u, goal %u, count %u\n", ucpi->c_cgx, goal, count))

uspi = UFS_SB(sb)->s_uspi;
- usb1 = ubh_get_usb_first (USPI_UBH);
+ usb1 = ubh_get_usb_first (uspi);
ucg = ubh_get_ucg(UCPI_UBH);

if (goal)
diff -uprN -X linux-2.6.15-vanilla/Documentation/dontdiff linux-2.6.15-vanilla/fs/ufs/ialloc.c linux-2.6.15/fs/ufs/ialloc.c
--- linux-2.6.15-vanilla/fs/ufs/ialloc.c 2006-01-03 06:21:10.000000000 +0300
+++ linux-2.6.15/fs/ufs/ialloc.c 2006-01-14 09:50:06.924135000 +0300
@@ -72,7 +72,7 @@ void ufs_free_inode (struct inode * inod

sb = inode->i_sb;
uspi = UFS_SB(sb)->s_uspi;
- usb1 = ubh_get_usb_first(USPI_UBH);
+ usb1 = ubh_get_usb_first(uspi);

ino = inode->i_ino;

@@ -167,7 +167,7 @@ struct inode * ufs_new_inode(struct inod
ufsi = UFS_I(inode);
sbi = UFS_SB(sb);
uspi = sbi->s_uspi;
- usb1 = ubh_get_usb_first(USPI_UBH);
+ usb1 = ubh_get_usb_first(uspi);

lock_super (sb);

diff -uprN -X linux-2.6.15-vanilla/Documentation/dontdiff linux-2.6.15-vanilla/fs/ufs/inode.c linux-2.6.15/fs/ufs/inode.c
--- linux-2.6.15-vanilla/fs/ufs/inode.c 2006-01-03 06:21:10.000000000 +0300
+++ linux-2.6.15/fs/ufs/inode.c 2006-01-14 09:50:06.964137500 +0300
@@ -61,7 +61,7 @@ static int ufs_block_to_path(struct inod
int n = 0;


- UFSD(("ptrs=uspi->s_apb = %d,double_blocks=%d \n",ptrs,double_blocks));
+ UFSD(("ptrs=uspi->s_apb = %d,double_blocks=%ld \n",ptrs,double_blocks));
if (i_block < 0) {
ufs_warning(inode->i_sb, "ufs_block_to_path", "block < 0");
} else if (i_block < direct_blocks) {
@@ -104,7 +104,7 @@ u64 ufs_frag_map(struct inode *inode, s
unsigned flags = UFS_SB(sb)->s_flags;
u64 temp = 0L;

- UFSD((": frag = %lu depth = %d\n",frag,depth));
+ UFSD((": frag = %llu depth = %d\n", (unsigned long long)frag, depth));
UFSD((": uspi->s_fpbshift = %d ,uspi->s_apbmask = %x, mask=%llx\n",uspi->s_fpbshift,uspi->s_apbmask,mask));

if (depth == 0)
@@ -365,9 +365,10 @@ repeat:
sync_dirty_buffer(bh);
inode->i_ctime = CURRENT_TIME_SEC;
mark_inode_dirty(inode);
+ UFSD(("result %u\n", tmp + blockoff));
out:
brelse (bh);
- UFSD(("EXIT, result %u\n", tmp + blockoff))
+ UFSD(("EXIT\n"));
return result;
}

@@ -386,7 +387,7 @@ static int ufs_getfrag_block (struct ino

if (!create) {
phys64 = ufs_frag_map(inode, fragment);
- UFSD(("phys64 = %lu \n",phys64));
+ UFSD(("phys64 = %llu \n",phys64));
if (phys64)
map_bh(bh_result, sb, phys64);
return 0;
@@ -401,7 +402,7 @@ static int ufs_getfrag_block (struct ino

lock_kernel();

- UFSD(("ENTER, ino %lu, fragment %u\n", inode->i_ino, fragment))
+ UFSD(("ENTER, ino %lu, fragment %llu\n", inode->i_ino, (unsigned long long)fragment))
if (fragment < 0)
goto abort_negative;
if (fragment >
diff -uprN -X linux-2.6.15-vanilla/Documentation/dontdiff linux-2.6.15-vanilla/fs/ufs/super.c linux-2.6.15/fs/ufs/super.c
--- linux-2.6.15-vanilla/fs/ufs/super.c 2006-01-03 06:21:10.000000000 +0300
+++ linux-2.6.15/fs/ufs/super.c 2006-01-14 09:50:07.036142000 +0300
@@ -221,7 +221,7 @@ void ufs_error (struct super_block * sb,
va_list args;

uspi = UFS_SB(sb)->s_uspi;
- usb1 = ubh_get_usb_first(USPI_UBH);
+ usb1 = ubh_get_usb_first(uspi);

if (!(sb->s_flags & MS_RDONLY)) {
usb1->fs_clean = UFS_FSBAD;
@@ -253,7 +253,7 @@ void ufs_panic (struct super_block * sb,
va_list args;

uspi = UFS_SB(sb)->s_uspi;
- usb1 = ubh_get_usb_first(USPI_UBH);
+ usb1 = ubh_get_usb_first(uspi);

if (!(sb->s_flags & MS_RDONLY)) {
usb1->fs_clean = UFS_FSBAD;
@@ -420,21 +420,18 @@ static int ufs_read_cylinder_structures
if (i + uspi->s_fpb > blks)
size = (blks - i) * uspi->s_fsize;

- if ((flags & UFS_TYPE_MASK) == UFS_TYPE_UFS2) {
+ if ((flags & UFS_TYPE_MASK) == UFS_TYPE_UFS2)
ubh = ubh_bread(sb,
fs64_to_cpu(sb, usb->fs_u11.fs_u2.fs_csaddr) + i, size);
- if (!ubh)
- goto failed;
- ubh_ubhcpymem (space, ubh, size);
- sbi->s_csp[ufs_fragstoblks(i)]=(struct ufs_csum *)space;
- }
- else {
+ else
ubh = ubh_bread(sb, uspi->s_csaddr + i, size);
- if (!ubh)
- goto failed;
- ubh_ubhcpymem(space, ubh, size);
- sbi->s_csp[ufs_fragstoblks(i)]=(struct ufs_csum *)space;
- }
+
+ if (!ubh)
+ goto failed;
+
+ ubh_ubhcpymem (space, ubh, size);
+ sbi->s_csp[ufs_fragstoblks(i)]=(struct ufs_csum *)space;
+
space += size;
ubh_brelse (ubh);
ubh = NULL;
@@ -539,6 +536,7 @@ static int ufs_fill_super(struct super_b
struct inode *inode;
unsigned block_size, super_block_size;
unsigned flags;
+ unsigned super_block_offset;

uspi = NULL;
ubh = NULL;
@@ -586,10 +584,11 @@ static int ufs_fill_super(struct super_b
if (!uspi)
goto failed;

+ super_block_offset=UFS_SBLOCK;
+
/* Keep 2Gig file limit. Some UFS variants need to override
this but as I don't know which I'll let those in the know loosen
the rules */
-
switch (sbi->s_mount_opt & UFS_MOUNT_UFSTYPE) {
case UFS_MOUNT_UFSTYPE_44BSD:
UFSD(("ufstype=44bsd\n"))
@@ -601,7 +600,8 @@ static int ufs_fill_super(struct super_b
flags |= UFS_DE_44BSD | UFS_UID_44BSD | UFS_ST_44BSD | UFS_CG_44BSD;
break;
case UFS_MOUNT_UFSTYPE_UFS2:
- UFSD(("ufstype=ufs2\n"))
+ UFSD(("ufstype=ufs2\n"));
+ super_block_offset=SBLOCK_UFS2;
uspi->s_fsize = block_size = 512;
uspi->s_fmask = ~(512 - 1);
uspi->s_fshift = 9;
@@ -725,19 +725,16 @@ again:
/*
* read ufs super block from device
*/
- if ( (flags & UFS_TYPE_MASK) == UFS_TYPE_UFS2) {
- ubh = ubh_bread_uspi(uspi, sb, uspi->s_sbbase + SBLOCK_UFS2/block_size, super_block_size);
- }
- else {
- ubh = ubh_bread_uspi(uspi, sb, uspi->s_sbbase + UFS_SBLOCK/block_size, super_block_size);
- }
+
+ ubh = ubh_bread_uspi(uspi, sb, uspi->s_sbbase + super_block_offset/block_size, super_block_size);
+
if (!ubh)
goto failed;


- usb1 = ubh_get_usb_first(USPI_UBH);
- usb2 = ubh_get_usb_second(USPI_UBH);
- usb3 = ubh_get_usb_third(USPI_UBH);
+ usb1 = ubh_get_usb_first(uspi);
+ usb2 = ubh_get_usb_second(uspi);
+ usb3 = ubh_get_usb_third(uspi);
usb = (struct ufs_super_block *)
((struct ufs_buffer_head *)uspi)->bh[0]->b_data ;

@@ -1006,8 +1003,8 @@ static void ufs_write_super (struct supe
UFSD(("ENTER\n"))
flags = UFS_SB(sb)->s_flags;
uspi = UFS_SB(sb)->s_uspi;
- usb1 = ubh_get_usb_first(USPI_UBH);
- usb3 = ubh_get_usb_third(USPI_UBH);
+ usb1 = ubh_get_usb_first(uspi);
+ usb3 = ubh_get_usb_third(uspi);

if (!(sb->s_flags & MS_RDONLY)) {
usb1->fs_time = cpu_to_fs32(sb, get_seconds());
@@ -1049,8 +1046,8 @@ static int ufs_remount (struct super_blo

uspi = UFS_SB(sb)->s_uspi;
flags = UFS_SB(sb)->s_flags;
- usb1 = ubh_get_usb_first(USPI_UBH);
- usb3 = ubh_get_usb_third(USPI_UBH);
+ usb1 = ubh_get_usb_first(uspi);
+ usb3 = ubh_get_usb_third(uspi);

/*
* Allow the "check" option to be passed as a remount option.
@@ -1124,7 +1121,7 @@ static int ufs_statfs (struct super_bloc
lock_kernel();

uspi = UFS_SB(sb)->s_uspi;
- usb1 = ubh_get_usb_first (USPI_UBH);
+ usb1 = ubh_get_usb_first (uspi);
usb = (struct ufs_super_block *)
((struct ufs_buffer_head *)uspi)->bh[0]->b_data ;

diff -uprN -X linux-2.6.15-vanilla/Documentation/dontdiff linux-2.6.15-vanilla/fs/ufs/util.h linux-2.6.15/fs/ufs/util.h
--- linux-2.6.15-vanilla/fs/ufs/util.h 2006-01-14 09:47:20.429729750 +0300
+++ linux-2.6.15/fs/ufs/util.h 2006-01-14 09:59:08.261966500 +0300
@@ -249,18 +249,28 @@ extern void _ubh_memcpyubh_(struct ufs_s


/*
- * macros to get important structures from ufs_buffer_head
+ * macros and inline function to get important structures from ufs_sb_private_info
*/
-#define ubh_get_usb_first(ubh) \
- ((struct ufs_super_block_first *)((ubh)->bh[0]->b_data))

-#define ubh_get_usb_second(ubh) \
- ((struct ufs_super_block_second *)((ubh)->\
- bh[UFS_SECTOR_SIZE >> uspi->s_fshift]->b_data + (UFS_SECTOR_SIZE & ~uspi->s_fmask)))
-
-#define ubh_get_usb_third(ubh) \
- ((struct ufs_super_block_third *)((ubh)-> \
- bh[UFS_SECTOR_SIZE*2 >> uspi->s_fshift]->b_data + (UFS_SECTOR_SIZE*2 & ~uspi->s_fmask)))
+static inline void *get_usb_offset(struct ufs_sb_private_info *uspi,
+ unsigned int offset)
+{
+ unsigned int index;
+
+ index = offset >> uspi->s_fshift;
+ offset &= ~uspi->s_fmask;
+ return uspi->s_ubh.bh[index]->b_data + offset;
+}
+
+#define ubh_get_usb_first(uspi) \
+ ((struct ufs_super_block_first *)get_usb_offset((uspi), 0))
+
+#define ubh_get_usb_second(uspi) \
+ ((struct ufs_super_block_second *)get_usb_offset((uspi), UFS_SECTOR_SIZE))
+
+#define ubh_get_usb_third(uspi) \
+ ((struct ufs_super_block_third *)get_usb_offset((uspi), 2*UFS_SECTOR_SIZE))
+

#define ubh_get_ucg(ubh) \
((struct ufs_cylinder_group *)((ubh)->bh[0]->b_data))

--
/Evgeniy