--- On Tue, 6/9/11, Pavel Ivanov <paivanof@xxxxxxxxx> wrote: > On Sat, Sep 3, 2011 at 8:37 PM, > Hin-Tak Leung <hintak_leung@xxxxxxxxxxx> > wrote: > >> I've looked into the code myself a little and > here's what I > >> see. > >> hfsplus_read_wrapper() calls to > hfsplus_submit_bio() twice > >> to fill > >> sbi->s_vhdr and sbi->s_backup_vhdr. And > according to > >> parameters they > >> are filled with pointers into sbi->s_vhdr_buf > and > >> sbi->s_backup_vhdr_buf respectively. It's done > with the > >> following code > >> at fs/hfsplus/wrapper.c:79: > >> > >> if (!(rw & WRITE) && data) > >> *data = (u8 *)buf + > >> offset; > >> > >> So s_vhdr and s_backup_vhdr shouldn't be freed, > s_vhdr_buf > >> and > >> s_backup_vhdr_buf should be freed instead. And > indeed > >> changing in > >> hfsplus_fill_super() > >> > >> kfree(sbi->s_vhdr); > >> kfree(sbi->s_backup_vhdr); > >> > >> to > >> > >> kfree(sbi->s_vhdr_buf); > >> kfree(sbi->s_backup_vhdr_buf); > >> > >> fixes BUG reports from SLUB. > > > > The code around there is a bit too dense, and both of > the *_buf are recent introductions (and temp values, I > think) as is hfsplus_submit_bio() itself, around the > 2.6.39/3.0 time frame. I think the intention is to fill > s_vhdr/s_backup_vhdr via mulitple fetches using *_buf as > temp buffer. > > Well, look at the commit 6596528e. It clearly shows that > result of > kmalloc() is no longer assigned to sbi->s_vhdr and > sbi->s_backup_vhdr, > but is assigned to sbi->s_vhdr_buf and > sbi->s_backup_vhdr_buf. Also > this commit clearly changes hfsplus_put_super() so that it > doesn't > free sbi->s_vhdr and sbi->s_backup_vhdr, but frees > sbi->s_vhdr_buf and > sbi->s_backup_vhdr_buf instead. I guess Seth just > missed > hfsplus_fill_super() in there as it's pretty unusual exit > path. Indeed. But the differing role of the *vhdr and *buf wasn't clear. > >> Now, the problem with "too large" error is > trickier. > >> According to this message > >> > >> >> [ 92.549197] hfs: filesystem size > too large > >> blksz_shift=14, total_blocks=486494 > >> > >> HFS thinks that my iPod has block size of 16 KiB. > But > >> generic_check_addressable() decides that > everything with > >> blocks larger > >> than PAGE_CACHE_SHIFT (i.e. 4 KiB on my system) > cannot be > >> addressable > >> and thus filesystem cannot be mounted. I guess it > wasn't > >> supposed to > >> be that way. Is hfsplus_read_wrapper() wrong in > determining > >> block size > >> or all iPods where this was tested actually had > block size > >> 4 KiB or > >> less? > > > > Your logical sector size is 4k according to dmesg and > hfs block size is 512 so that 16KiB is a bit dodgy. > > I'm not sure where that "logical sector size" of 4k comes > from but > according to the sources 16K is taken directly from iPod > via vhdr in > hfsplus_read_wrapper(). And apparently all hfsplus code is > designed to > work with blocks larger than PAGE_SIZE. So it's just > generic_check_addressable() that stands in the way. Maybe > commit > c6d5f5fa wasn't quite well thought through or tested by > Christoph? Yes, the 16k seem correct. The dmesg message is misleading. > Anyway the following patch worked for me and I've got my > iPod mounted > and navigateable (although only in read-only mode because > it has > journaled filesystem). I worked on the netgear journalling patches a bit: http://htl10.users.sourceforge.net/patchsets/hfsplus_3.0_rfc/patches/ But please *back up your data*, as well as reading my notes before trying them out: http://www.spinics.net/lists/linux-fsdevel/msg47684.html http://www.spinics.net/lists/linux-fsdevel/msg47734.html > diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c > index c106ca2..5458be4 100644 > --- a/fs/hfsplus/super.c > +++ b/fs/hfsplus/super.c > @@ -345,6 +345,8 @@ static int hfsplus_fill_super(struct > super_block > *sb, void *data, int silent) > struct qstr str; > struct nls_table *nls = NULL; > int err; > + unsigned check_blksz_bits; > + u64 check_num_blocks; > > err = -EINVAL; > sbi = kzalloc(sizeof(*sbi), > GFP_KERNEL); > @@ -399,10 +401,15 @@ static int hfsplus_fill_super(struct > super_block > *sb, void *data, int silent) > if (!sbi->rsrc_clump_blocks) > > sbi->rsrc_clump_blocks = 1; > > - err = > generic_check_addressable(sbi->alloc_blksz_shift, > - > > sbi->total_blocks); > + check_blksz_bits = > sbi->alloc_blksz_shift; > + check_num_blocks = > sbi->total_blocks; > + if (sbi->fs_shift != 0) { > + check_num_blocks > <<= sbi->fs_shift; > + check_blksz_bits -= > sbi->fs_shift; > + } > + err = > generic_check_addressable(check_blksz_bits, > check_num_blocks); > if (err) { > printk(KERN_ERR "hfs: > filesystem size too large.\n"); > goto out_free_vhdr; > > > I can format and submit both patches (plus a small cleanup > that I felt > is needed to be changed along the way). Tell me what you > think. This is a bit ugly - what it does is massage the two values taking into account of sbi->fs_shift to pass check_addressable() . I think either check_addressable should be extended, or this be abstracted out say, to check_hfs_addressable() (with __inline__ if desired) to be cleaner. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
