On Wed, Sep 07, 2011 at 02:26:54PM +0200, Lukas Czerner wrote:
> I do not think that start + len can overflow since we are doing
> XFS_B_TO_FSBT() on it first. Am I missing something ?
They don't have to overflow, but they can easily be outside
the range of valid AGs.
> > Care to update the test case to cover these cases as well?
> >
>
> I am not sure what do you mean ? There already is a check when both
> start and len are huge numbers. I am not sure if we can do more without
> significantly complicating the test to cover various start, or len
> numbers where can the fsblock->group_number overflow for various file
> systems.
Add a testcase where start is a relatively small number (smaller than an
AG/BG), but start + len is outside the fs.
> @@ -145,7 +145,7 @@ xfs_ioc_trim(
> struct request_queue *q = mp->m_ddev_targp->bt_bdev->bd_disk->queue;
> unsigned int granularity = q->limits.discard_granularity;
> struct fstrim_range range;
> + xfs_fsblock_t start, end, minlen;
> xfs_agnumber_t start_agno, end_agno, agno;
> __uint64_t blocks_trimmed = 0;
> int error, last_error = 0;
> @@ -165,19 +165,21 @@ xfs_ioc_trim(
> * matter as trimming blocks is an advisory interface.
> */
> start = XFS_B_TO_FSBT(mp, range.start);
> + end = start + XFS_B_TO_FSBT(mp, range.len) - 1;
> minlen = XFS_B_TO_FSB(mp, max_t(u64, granularity, range.minlen));
>
> + if (start >= mp->m_sb.sb_dblocks)
> return -XFS_ERROR(EINVAL);
> + start_agno = XFS_FSB_TO_AGNO(mp, start);
>
> + if (end >= mp->m_sb.sb_dblocks) {
> + end = mp->m_sb.sb_dblocks - 1;
> end_agno = mp->m_sb.sb_agcount - 1;
> + } else
> + end_agno = XFS_FSB_TO_AGNO(mp, end);
I'd rather do something like:
if (start >= mp->m_sb.sb_dblocks)
return -XFS_ERROR(EINVAL);
if (end > mp->m_sb.sb_dblocks - 1)
end = mp->m_sb.sb_dblocks - 1;
start_agno = XFS_FSB_TO_AGNO(mp, start);
end_agno = XFS_FSB_TO_AGNO(mp, end)
here.
Otherwise the patch looks fine.
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
