* [PATCH] kill BMAPI_DEVICE
@ 2007-09-09 15:39 Christoph Hellwig
2007-09-09 19:32 ` Bhagi rathi
0 siblings, 1 reply; 4+ messages in thread
From: Christoph Hellwig @ 2007-09-09 15:39 UTC (permalink / raw)
To: xfs
There is no reason to go into the iomap machinery just to get the
right block device for an inode. Instead look at the realtime flag
in the inode and grab the right device from the mount structure.
I created a new helper, xfs_find_bdev_for_inode instead of opencoding
it because I plan to use it in other places in the future.
Signed-off-by: Christoph Hellwig <hch@lst.de>
Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_aops.c
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_aops.c 2007-08-23 14:54:01.000000000 +0200
+++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_aops.c 2007-08-23 14:59:55.000000000 +0200
@@ -107,6 +107,18 @@ xfs_page_trace(
#define xfs_page_trace(tag, inode, page, pgoff)
#endif
+STATIC struct block_device *
+xfs_find_bdev_for_inode(
+ struct xfs_inode *ip)
+{
+ struct xfs_mount *mp = ip->i_mount;
+
+ if (ip->i_d.di_flags & XFS_DIFLAG_REALTIME)
+ return mp->m_rtdev_targp->bt_bdev;
+ else
+ return mp->m_ddev_targp->bt_bdev;
+}
+
/*
* Schedule IO completion handling on a xfsdatad if this was
* the final hold on this ioend. If we are asked to wait,
@@ -1476,28 +1488,21 @@ xfs_vm_direct_IO(
{
struct file *file = iocb->ki_filp;
struct inode *inode = file->f_mapping->host;
- xfs_iomap_t iomap;
- int maps = 1;
- int error;
+ struct block_device *bdev;
ssize_t ret;
- error = xfs_bmap(XFS_I(inode), offset, 0,
- BMAPI_DEVICE, &iomap, &maps);
- if (error)
- return -error;
+ bdev = xfs_find_bdev_for_inode(XFS_I(inode));
if (rw == WRITE) {
iocb->private = xfs_alloc_ioend(inode, IOMAP_UNWRITTEN);
ret = blockdev_direct_IO_own_locking(rw, iocb, inode,
- iomap.iomap_target->bt_bdev,
- iov, offset, nr_segs,
+ bdev, iov, offset, nr_segs,
xfs_get_blocks_direct,
xfs_end_io_direct);
} else {
iocb->private = xfs_alloc_ioend(inode, IOMAP_READ);
ret = blockdev_direct_IO_no_locking(rw, iocb, inode,
- iomap.iomap_target->bt_bdev,
- iov, offset, nr_segs,
+ bdev, iov, offset, nr_segs,
xfs_get_blocks_direct,
xfs_end_io_direct);
}
Index: linux-2.6-xfs/fs/xfs/xfs_iomap.c
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/xfs_iomap.c 2007-08-23 14:39:45.000000000 +0200
+++ linux-2.6-xfs/fs/xfs/xfs_iomap.c 2007-08-23 14:59:55.000000000 +0200
@@ -193,7 +193,7 @@ xfs_iomap(
switch (flags &
(BMAPI_READ | BMAPI_WRITE | BMAPI_ALLOCATE |
- BMAPI_UNWRITTEN | BMAPI_DEVICE)) {
+ BMAPI_UNWRITTEN)) {
case BMAPI_READ:
xfs_iomap_enter_trace(XFS_IOMAP_READ_ENTER, io, offset, count);
lockmode = XFS_LCK_MAP_SHARED(mp, io);
@@ -220,13 +220,6 @@ xfs_iomap(
break;
case BMAPI_UNWRITTEN:
goto phase2;
- case BMAPI_DEVICE:
- lockmode = XFS_LCK_MAP_SHARED(mp, io);
- iomapp->iomap_target = io->io_flags & XFS_IOCORE_RT ?
- mp->m_rtdev_targp : mp->m_ddev_targp;
- error = 0;
- *niomaps = 1;
- goto out;
default:
BUG();
}
Index: linux-2.6-xfs/fs/xfs/xfs_iomap.h
===================================================================
--- linux-2.6-xfs.orig/fs/xfs/xfs_iomap.h 2007-08-23 14:39:45.000000000 +0200
+++ linux-2.6-xfs/fs/xfs/xfs_iomap.h 2007-08-23 14:59:55.000000000 +0200
@@ -43,7 +43,6 @@ typedef enum {
BMAPI_MMAP = (1 << 6), /* allocate for mmap write */
BMAPI_SYNC = (1 << 7), /* sync write to flush delalloc space */
BMAPI_TRYLOCK = (1 << 8), /* non-blocking request */
- BMAPI_DEVICE = (1 << 9), /* we only want to know the device */
} bmapi_flags_t;
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] kill BMAPI_DEVICE
2007-09-09 15:39 [PATCH] kill BMAPI_DEVICE Christoph Hellwig
@ 2007-09-09 19:32 ` Bhagi rathi
2007-09-10 12:01 ` Christoph Hellwig
0 siblings, 1 reply; 4+ messages in thread
From: Bhagi rathi @ 2007-09-09 19:32 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
XFS_IOCORE_RT | XFS_DIFLAG_REALTIME can be set from an ioctl (xfs_setattr).
A directIO without holding ILOCK
in shared in mode can read a wrong value of di_flag for real time decision.
As a result, we may pass in-correct device
during directIO as the proposed xfs_find_bdev_for_inode doesn't hold any
lock in reading the flags. It is not based
on iocore flags as well.
On a secondary note, XFS_IOCORE_RT was set without holding iolock which
seems to an issue. I tend to leave
xfs_bmapi to decide BMAPI_DEVICE to xfs_iomap.
What is the reason why this has to be seperated?
Thanks,
-Saradhi.
On 9/9/07, Christoph Hellwig <hch@lst.de> wrote:
>
> There is no reason to go into the iomap machinery just to get the
> right block device for an inode. Instead look at the realtime flag
> in the inode and grab the right device from the mount structure.
>
> I created a new helper, xfs_find_bdev_for_inode instead of opencoding
> it because I plan to use it in other places in the future.
>
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_aops.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_aops.c 2007-08-23 14:54:
> 01.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_aops.c 2007-08-23 14:59:
> 55.000000000 +0200
> @@ -107,6 +107,18 @@ xfs_page_trace(
> #define xfs_page_trace(tag, inode, page, pgoff)
> #endif
>
> +STATIC struct block_device *
> +xfs_find_bdev_for_inode(
> + struct xfs_inode *ip)
> +{
> + struct xfs_mount *mp = ip->i_mount;
> +
> + if (ip->i_d.di_flags & XFS_DIFLAG_REALTIME)
> + return mp->m_rtdev_targp->bt_bdev;
> + else
> + return mp->m_ddev_targp->bt_bdev;
> +}
> +
> /*
> * Schedule IO completion handling on a xfsdatad if this was
> * the final hold on this ioend. If we are asked to wait,
> @@ -1476,28 +1488,21 @@ xfs_vm_direct_IO(
> {
> struct file *file = iocb->ki_filp;
> struct inode *inode = file->f_mapping->host;
> - xfs_iomap_t iomap;
> - int maps = 1;
> - int error;
> + struct block_device *bdev;
> ssize_t ret;
>
> - error = xfs_bmap(XFS_I(inode), offset, 0,
> - BMAPI_DEVICE, &iomap, &maps);
> - if (error)
> - return -error;
> + bdev = xfs_find_bdev_for_inode(XFS_I(inode));
>
> if (rw == WRITE) {
> iocb->private = xfs_alloc_ioend(inode, IOMAP_UNWRITTEN);
> ret = blockdev_direct_IO_own_locking(rw, iocb, inode,
> - iomap.iomap_target->bt_bdev,
> - iov, offset, nr_segs,
> + bdev, iov, offset, nr_segs,
> xfs_get_blocks_direct,
> xfs_end_io_direct);
> } else {
> iocb->private = xfs_alloc_ioend(inode, IOMAP_READ);
> ret = blockdev_direct_IO_no_locking(rw, iocb, inode,
> - iomap.iomap_target->bt_bdev,
> - iov, offset, nr_segs,
> + bdev, iov, offset, nr_segs,
> xfs_get_blocks_direct,
> xfs_end_io_direct);
> }
> Index: linux-2.6-xfs/fs/xfs/xfs_iomap.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/xfs_iomap.c 2007-08-23 14:39:
> 45.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/xfs_iomap.c 2007-08-23 14:59:55.000000000+0200
> @@ -193,7 +193,7 @@ xfs_iomap(
>
> switch (flags &
> (BMAPI_READ | BMAPI_WRITE | BMAPI_ALLOCATE |
> - BMAPI_UNWRITTEN | BMAPI_DEVICE)) {
> + BMAPI_UNWRITTEN)) {
> case BMAPI_READ:
> xfs_iomap_enter_trace(XFS_IOMAP_READ_ENTER, io, offset,
> count);
> lockmode = XFS_LCK_MAP_SHARED(mp, io);
> @@ -220,13 +220,6 @@ xfs_iomap(
> break;
> case BMAPI_UNWRITTEN:
> goto phase2;
> - case BMAPI_DEVICE:
> - lockmode = XFS_LCK_MAP_SHARED(mp, io);
> - iomapp->iomap_target = io->io_flags & XFS_IOCORE_RT ?
> - mp->m_rtdev_targp : mp->m_ddev_targp;
> - error = 0;
> - *niomaps = 1;
> - goto out;
> default:
> BUG();
> }
> Index: linux-2.6-xfs/fs/xfs/xfs_iomap.h
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/xfs_iomap.h 2007-08-23 14:39:
> 45.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/xfs_iomap.h 2007-08-23 14:59:55.000000000+0200
> @@ -43,7 +43,6 @@ typedef enum {
> BMAPI_MMAP = (1 << 6), /* allocate for mmap write */
> BMAPI_SYNC = (1 << 7), /* sync write to flush delalloc
> space */
> BMAPI_TRYLOCK = (1 << 8), /* non-blocking request */
> - BMAPI_DEVICE = (1 << 9), /* we only want to know the device
> */
> } bmapi_flags_t;
>
>
>
>
>
[[HTML alternate version deleted]]
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] kill BMAPI_DEVICE
2007-09-09 19:32 ` Bhagi rathi
@ 2007-09-10 12:01 ` Christoph Hellwig
2007-09-10 13:20 ` Bhagi rathi
0 siblings, 1 reply; 4+ messages in thread
From: Christoph Hellwig @ 2007-09-10 12:01 UTC (permalink / raw)
To: Bhagi rathi; +Cc: Christoph Hellwig, xfs
On Mon, Sep 10, 2007 at 01:02:07AM +0530, Bhagi rathi wrote:
> XFS_IOCORE_RT | XFS_DIFLAG_REALTIME can be set from an ioctl (xfs_setattr).
> A directIO without holding ILOCK
> in shared in mode can read a wrong value of di_flag for real time decision.
> As a result, we may pass in-correct device
> during directIO as the proposed xfs_find_bdev_for_inode doesn't hold any
> lock in reading the flags. It is not based
> on iocore flags as well.
>
> On a secondary note, XFS_IOCORE_RT was set without holding iolock which
> seems to an issue. I tend to leave
> xfs_bmapi to decide BMAPI_DEVICE to xfs_iomap.
The previous locking doesn't help anything - if the value changes during
the direct I/O process we are in trouble anyway. Fortunately we always
hold the iolock in shared mode over any direct I/O, so taking this lock
in ->setattr will fix this pre-existing issue.
> What is the reason why this has to be seperated?
Because it does not belong into xfs_iomap. While there is at least some
common code between BMAPI_READ, BMAPI_WRITE and BMAPI_ALLOCATE calls so
sharing that makes some sense it does not at all for the other two which
this patch separates out in preparation of bigger changes in this area.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] kill BMAPI_DEVICE
2007-09-10 12:01 ` Christoph Hellwig
@ 2007-09-10 13:20 ` Bhagi rathi
0 siblings, 0 replies; 4+ messages in thread
From: Bhagi rathi @ 2007-09-10 13:20 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On 9/10/07, Christoph Hellwig <hch@lst.de> wrote:
>
> On Mon, Sep 10, 2007 at 01:02:07AM +0530, Bhagi rathi wrote:
> > XFS_IOCORE_RT | XFS_DIFLAG_REALTIME can be set from an ioctl
> (xfs_setattr).
> > A directIO without holding ILOCK
> > in shared in mode can read a wrong value of di_flag for real time
> decision.
> > As a result, we may pass in-correct device
> > during directIO as the proposed xfs_find_bdev_for_inode doesn't hold any
> > lock in reading the flags. It is not based
> > on iocore flags as well.
> >
> > On a secondary note, XFS_IOCORE_RT was set without holding iolock which
> > seems to an issue. I tend to leave
> > xfs_bmapi to decide BMAPI_DEVICE to xfs_iomap.
>
> The previous locking doesn't help anything - if the value changes during
> the direct I/O process we are in trouble anyway. Fortunately we always
> hold the iolock in shared mode over any direct I/O, so taking this lock
> in ->setattr will fix this pre-existing issue.
Yes. I believe the proposed change can succeed after the fix you mentioned.
> What is the reason why this has to be seperated?
>
> Because it does not belong into xfs_iomap. While there is at least some
> common code between BMAPI_READ, BMAPI_WRITE and BMAPI_ALLOCATE calls so
> sharing that makes some sense it does not at all for the other two which
> this patch separates out in preparation of bigger changes in this area.
What is the goal of the bigger changes planned?
-Saradhi.
[[HTML alternate version deleted]]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2007-09-10 13:20 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-09-09 15:39 [PATCH] kill BMAPI_DEVICE Christoph Hellwig
2007-09-09 19:32 ` Bhagi rathi
2007-09-10 12:01 ` Christoph Hellwig
2007-09-10 13:20 ` Bhagi rathi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox