From: Linus Torvalds <torvalds@linux-foundation.org>
To: Josef Bacik <josef@redhat.com>
Cc: akpm@linux-foundation.org, linux-fsdevel@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH] cleanup __generic_block_fiemap and fix a few minor issues
Date: Thu, 4 Mar 2010 08:03:55 -0800 (PST) [thread overview]
Message-ID: <alpine.LFD.2.00.1003040754150.7046@localhost.localdomain> (raw)
In-Reply-To: <20100303163715.GB2215@localhost.localdomain>
On Wed, 3 Mar 2010, Josef Bacik wrote:
>
> 1) Some filesystems (gfs2) do not like being given non-block-aligned b_size's to
> map, so make sure we block align the b_size.
As far as I can tell, that alignment is wrong.
You can't just align "len". If "start" wasn't aligned, then just aligning
"len" is wrong. And if start is guaranteed to be aligned, how?
Also, the alignment itself is ugly as hell, doing
> + len = (len + (1 << inode->i_blkbits) - 1) &
> + ~((1 << inode->i_blkbits) - 1);
> + map_end = start + len;
When what you probably should have done is to do
map_end = start + len;
_before_ rounding 'start', and then after that you can now map 'start' and
'map_end' just using the regular "logical_to_blk" that you already use (up
or down depending on whether the thing should be inclusive or exclusive of
partial blocks). No?
IOW, the whole thing should probably look something like
sector_t start_blk, last_blk;
start_blk = logical_to_blk(inode, start);
last_blk = logical_to_blk(inode, start + len - 1);
and you're done. Short, sweet, and to the point (note that "last_blk"
really is the _last_ block, so if you want the length in blocks, it would
be "last_blk - start_blk + 1").
Linus
next prev parent reply other threads:[~2010-03-04 16:03 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-03-03 16:37 [PATCH] cleanup __generic_block_fiemap and fix a few minor issues Josef Bacik
2010-03-04 16:03 ` Linus Torvalds [this message]
2010-03-04 16:20 ` Josef Bacik
2010-03-04 16:50 ` Randy Dunlap
2010-03-04 17:12 ` Josef Bacik
2010-03-04 17:23 ` Randy Dunlap
2010-03-29 18:05 ` Josef Bacik
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.LFD.2.00.1003040754150.7046@localhost.localdomain \
--to=torvalds@linux-foundation.org \
--cc=akpm@linux-foundation.org \
--cc=josef@redhat.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).