| Message ID | 1452230879-18117-2-git-send-email-ross.zwisler@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu 07-01-16 22:27:51, Ross Zwisler wrote: > In __dax_pmd_fault() we currently assume that get_block() will always set > bh.b_bdev and we unconditionally dereference it in __dax_dbg(). This > assumption isn't always true - when called for reads of holes > ext4_dax_mmap_get_block() returns a buffer head where bh->b_bdev is never > set. I hit this BUG while testing the DAX PMD fault path. > > Instead, initialize bh.b_bdev before passing bh into get_block(). It is > possible that the filesystem's get_block() will update bh.b_bdev, and this > is fine - we just want to initialize bh.b_bdev to something reasonable so > that the calls to __dax_dbg() work and print something useful. > > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> > Cc: Dan Williams <dan.j.williams@intel.com> Looks good. But don't you need to do the same for __dax_fault(), dax_zero_page_range() and similar places passing bh to dax functions? Honza > --- > fs/dax.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/fs/dax.c b/fs/dax.c > index 7af8797..513bba5 100644 > --- a/fs/dax.c > +++ b/fs/dax.c > @@ -624,6 +624,7 @@ int __dax_pmd_fault(struct vm_area_struct *vma, unsigned long address, > } > > memset(&bh, 0, sizeof(bh)); > + bh.b_bdev = inode->i_sb->s_bdev; > block = (sector_t)pgoff << (PAGE_SHIFT - blkbits); > > bh.b_size = PMD_SIZE; > -- > 2.5.0 > >
On Tue, Jan 12, 2016 at 10:34:58AM +0100, Jan Kara wrote: > On Thu 07-01-16 22:27:51, Ross Zwisler wrote: > > In __dax_pmd_fault() we currently assume that get_block() will always set > > bh.b_bdev and we unconditionally dereference it in __dax_dbg(). This > > assumption isn't always true - when called for reads of holes > > ext4_dax_mmap_get_block() returns a buffer head where bh->b_bdev is never > > set. I hit this BUG while testing the DAX PMD fault path. > > > > Instead, initialize bh.b_bdev before passing bh into get_block(). It is > > possible that the filesystem's get_block() will update bh.b_bdev, and this > > is fine - we just want to initialize bh.b_bdev to something reasonable so > > that the calls to __dax_dbg() work and print something useful. > > > > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> > > Cc: Dan Williams <dan.j.williams@intel.com> > > Looks good. But don't you need to do the same for __dax_fault(), > dax_zero_page_range() and similar places passing bh to dax functions? > > Honza I don't think we need it anywhere else. The only reason that we need to initialize the bh.b_bdev manually in the __dax_pmd_fault() path is that if the get_block() call ends up finding a hole (so doesn't fill out b_bdev) we still go through the dax_pmd_dbg() path to print an error message, which uses b_bdev. I believe that in the other paths where we hit a hole, such as __dax_fault(), we don't use b_bdev because we don't have the same error path prints, and the regular code for handling holes doesn't use b_bdev. That being said, if you feel like it's cleaner to initialize it everywhere so everything is consistent and we don't have to worry about it, I'm fine to make the change. > > --- > > fs/dax.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/fs/dax.c b/fs/dax.c > > index 7af8797..513bba5 100644 > > --- a/fs/dax.c > > +++ b/fs/dax.c > > @@ -624,6 +624,7 @@ int __dax_pmd_fault(struct vm_area_struct *vma, unsigned long address, > > } > > > > memset(&bh, 0, sizeof(bh)); > > + bh.b_bdev = inode->i_sb->s_bdev; > > block = (sector_t)pgoff << (PAGE_SHIFT - blkbits); > > > > bh.b_size = PMD_SIZE; > > -- > > 2.5.0 > > > > > -- > Jan Kara <jack@suse.com> > SUSE Labs, CR -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed 13-01-16 00:08:29, Ross Zwisler wrote: > On Tue, Jan 12, 2016 at 10:34:58AM +0100, Jan Kara wrote: > > On Thu 07-01-16 22:27:51, Ross Zwisler wrote: > > > In __dax_pmd_fault() we currently assume that get_block() will always set > > > bh.b_bdev and we unconditionally dereference it in __dax_dbg(). This > > > assumption isn't always true - when called for reads of holes > > > ext4_dax_mmap_get_block() returns a buffer head where bh->b_bdev is never > > > set. I hit this BUG while testing the DAX PMD fault path. > > > > > > Instead, initialize bh.b_bdev before passing bh into get_block(). It is > > > possible that the filesystem's get_block() will update bh.b_bdev, and this > > > is fine - we just want to initialize bh.b_bdev to something reasonable so > > > that the calls to __dax_dbg() work and print something useful. > > > > > > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> > > > Cc: Dan Williams <dan.j.williams@intel.com> > > > > Looks good. But don't you need to do the same for __dax_fault(), > > dax_zero_page_range() and similar places passing bh to dax functions? > > > > Honza > > I don't think we need it anywhere else. The only reason that we need to > initialize the bh.b_bdev manually in the __dax_pmd_fault() path is that if the > get_block() call ends up finding a hole (so doesn't fill out b_bdev) we still > go through the dax_pmd_dbg() path to print an error message, which uses > b_bdev. I believe that in the other paths where we hit a hole, such as > __dax_fault(), we don't use b_bdev because we don't have the same error path > prints, and the regular code for handling holes doesn't use b_bdev. > > That being said, if you feel like it's cleaner to initialize it > everywhere so everything is consistent and we don't have to worry about > it, I'm fine to make the change. Well, it seems more futureproof to me. In case someone decides to add some debug message later on... Honza
diff --git a/fs/dax.c b/fs/dax.c index 7af8797..513bba5 100644 --- a/fs/dax.c +++ b/fs/dax.c @@ -624,6 +624,7 @@ int __dax_pmd_fault(struct vm_area_struct *vma, unsigned long address, } memset(&bh, 0, sizeof(bh)); + bh.b_bdev = inode->i_sb->s_bdev; block = (sector_t)pgoff << (PAGE_SHIFT - blkbits); bh.b_size = PMD_SIZE;
In __dax_pmd_fault() we currently assume that get_block() will always set bh.b_bdev and we unconditionally dereference it in __dax_dbg(). This assumption isn't always true - when called for reads of holes ext4_dax_mmap_get_block() returns a buffer head where bh->b_bdev is never set. I hit this BUG while testing the DAX PMD fault path. Instead, initialize bh.b_bdev before passing bh into get_block(). It is possible that the filesystem's get_block() will update bh.b_bdev, and this is fine - we just want to initialize bh.b_bdev to something reasonable so that the calls to __dax_dbg() work and print something useful. Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com> Cc: Dan Williams <dan.j.williams@intel.com> --- fs/dax.c | 1 + 1 file changed, 1 insertion(+)