| Message ID | 1271863269-5423-1-git-send-email-kirill@shutemov.name |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2010-04-21 at 18:21 +0300, Kirill A. Shutemov wrote: > + /* > + * We cannot modify file->f_mapping->backing_dev_info directly, > + * because it will corrupt backing device for inode, since > + * inode->i_mapping is equal to file->f_mapping. So we have to > + * copy f_mapping first. > + */ > + file->f_mapping = kmalloc(sizeof(*file->f_mapping), GFP_KERNEL); > + memcpy(file->f_mapping, inode->i_mapping, > + sizeof(*file->f_mapping)); > file->f_mapping->backing_dev_info = mtd->backing_dev_info; > + } Ick. What about the rest of file->f_mapping? That'll still be inherited. Jan pointed at drivers/char/raw.c as an example, but that doesn't do anything as ugly as this -- that sets file->f_mapping to point at bdev->bd_inode->i_mapping instead. I suspect we should do something similar -- have an inode for the MTD device, with a valid i_data of its own.
On Thu 22-04-10 12:08:55, David Woodhouse wrote: > On Wed, 2010-04-21 at 18:21 +0300, Kirill A. Shutemov wrote: > > + /* > > + * We cannot modify file->f_mapping->backing_dev_info directly, > > + * because it will corrupt backing device for inode, since > > + * inode->i_mapping is equal to file->f_mapping. So we have to > > + * copy f_mapping first. > > + */ > > + file->f_mapping = kmalloc(sizeof(*file->f_mapping), GFP_KERNEL); > > + memcpy(file->f_mapping, inode->i_mapping, > > + sizeof(*file->f_mapping)); > > file->f_mapping->backing_dev_info = mtd->backing_dev_info; > > + } > > Ick. What about the rest of file->f_mapping? That'll still be inherited. > > Jan pointed at drivers/char/raw.c as an example, but that doesn't do > anything as ugly as this -- that sets file->f_mapping to point at > bdev->bd_inode->i_mapping instead. > > I suspect we should do something similar -- have an inode for the MTD > device, with a valid i_data of its own. Yes, that's what I meant by my suggestion... Sorry if I wasn't clear enough. Honza
diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c index 5b081cb..d85be4d 100644 --- a/drivers/mtd/mtdchar.c +++ b/drivers/mtd/mtdchar.c @@ -88,8 +88,23 @@ static int mtd_open(struct inode *inode, struct file *file) goto out; } - if (mtd->backing_dev_info) + if (mtd->backing_dev_info) { + /* + * We assume that file->f_mapping is equal to inode->i_mapping. + */ + BUG_ON(file->f_mapping != inode->i_mapping); + + /* + * We cannot modify file->f_mapping->backing_dev_info directly, + * because it will corrupt backing device for inode, since + * inode->i_mapping is equal to file->f_mapping. So we have to + * copy f_mapping first. + */ + file->f_mapping = kmalloc(sizeof(*file->f_mapping), GFP_KERNEL); + memcpy(file->f_mapping, inode->i_mapping, + sizeof(*file->f_mapping)); file->f_mapping->backing_dev_info = mtd->backing_dev_info; + } /* You can't open it RW if it's not a writeable device */ if ((file->f_mode & FMODE_WRITE) && !(mtd->flags & MTD_WRITEABLE)) { @@ -129,6 +144,11 @@ static int mtd_close(struct inode *inode, struct file *file) file->private_data = NULL; kfree(mfi); + if (mtd->backing_dev_info) { + kfree(file->f_mapping); + file->f_mapping = inode->i_mapping; + } + return 0; } /* mtd_close */
We cannot modify file->f_mapping->backing_dev_info directly, because it will corrupt backing device for inode, since inode->i_mapping == file->f_mapping (see __dentry_open() in fs/open.c). So we have to copy f_mapping first. Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name> --- drivers/mtd/mtdchar.c | 22 +++++++++++++++++++++- 1 files changed, 21 insertions(+), 1 deletions(-)