On Thu, Feb 10, 2005 at 02:59:32PM +0000, Anton Altaparmakov wrote:
> On Thu, 2005-02-10 at 15:50 +0100, Christoph Hellwig wrote:
> > On Thu, Feb 10, 2005 at 02:48:26PM +0000, Anton Altaparmakov wrote:
> > > If the igrab() were not done, it would be possible for clear_inode to be
> > > called on the 'parent' inode whilst at the same time one or more attr
> > > inodes (belonging to this 'parent') are in use and Bad Things(TM) would
> > > happen...
> >
> > What bad thing specificly? If there's shared information we should
> > probably refcount them separately.
>
> Each attr inode stores a pointer to its parent inode in NTFS_I(struct
> inode *vi)->ext.base_ntfs_ino. This pointer would point to random
> memory if clear_inode is called on the parent whilst the attr inode is
> still in use.
Ok. Al Viro suggested to put a spinlock around the bmp_ino pointer and
then igrab the master inode when accessing it, iput it when done.
Below is a rather half-backed patch to implement the suggestion. It
compiles, but I'm pretty sure I introduced various bugs.
Also the new spinlock isn't nice, it'd probably be better to reuse some
other lock for this single field.
--- 1.85/fs/ntfs/dir.c 2004-11-05 13:48:35 +01:00
+++ edited/fs/ntfs/dir.c 2005-02-13 17:34:51 +01:00
@@ -1102,7 +1102,7 @@ static int ntfs_readdir(struct file *fil
{
s64 ia_pos, ia_start, prev_ia_pos, bmp_pos;
loff_t fpos;
- struct inode *bmp_vi, *vdir = filp->f_dentry->d_inode;
+ struct inode *bmp_vi = NULL, *vdir = filp->f_dentry->d_inode;
struct super_block *sb = vdir->i_sb;
ntfs_inode *ndir = NTFS_I(vdir);
ntfs_volume *vol = NTFS_SB(sb);
@@ -1250,17 +1250,14 @@ skip_index_root:
/* Get the offset into the index allocation attribute. */
ia_pos = (s64)fpos - vol->mft_record_size;
ia_mapping = vdir->i_mapping;
- bmp_vi = ndir->itype.index.bmp_ino;
+
+ bmp_vi = ntfs_grab_bmp_inode(vdir);
if (unlikely(!bmp_vi)) {
- ntfs_debug("Inode 0x%lx, regetting index bitmap.", vdir->i_ino);
- bmp_vi = ntfs_attr_iget(vdir, AT_BITMAP, I30, 4);
- if (IS_ERR(bmp_vi)) {
- ntfs_error(sb, "Failed to get bitmap attribute.");
- err = PTR_ERR(bmp_vi);
- goto err_out;
- }
- ndir->itype.index.bmp_ino = bmp_vi;
+ ntfs_error(sb, "Failed to get bitmap attribute.");
+ err = -EINVAL;
+ goto err_out;
}
+
bmp_mapping = bmp_vi->i_mapping;
/* Get the starting bitmap bit position and sanity check it. */
bmp_pos = ia_pos >> ndir->itype.index.block_size_bits;
@@ -1445,6 +1442,8 @@ EOD:
abort:
kfree(name);
done:
+ if (bmp_vi)
+ ntfs_ungrab_bmp_inode(vdir);
#ifdef DEBUG
if (!rc)
ntfs_debug("EOD, fpos 0x%llx, returning 0.", fpos);
@@ -1455,6 +1454,8 @@ done:
filp->f_pos = fpos;
return 0;
err_out:
+ if (bmp_vi)
+ ntfs_ungrab_bmp_inode(vdir);
if (bmp_page)
ntfs_unmap_page(bmp_page);
if (ia_page) {
@@ -1537,8 +1538,16 @@ static int ntfs_dir_fsync(struct file *f
ntfs_debug("Entering for inode 0x%lx.", vi->i_ino);
BUG_ON(!S_ISDIR(vi->i_mode));
- if (NInoIndexAllocPresent(ni) && ni->itype.index.bmp_ino)
- write_inode_now(ni->itype.index.bmp_ino, !datasync);
+
+ if (NInoIndexAllocPresent(ni)) {
+ struct inode *bmp_inode = ntfs_grab_bmp_inode(vi);
+
+ if (bmp_inode) {
+ write_inode_now(bmp_inode, !datasync);
+ ntfs_ungrab_bmp_inode(vi);
+ }
+ }
+
ret = ntfs_write_inode(vi, 1);
write_inode_now(vi, !datasync);
err = sync_blockdev(vi->i_sb->s_bdev);
--- 1.154/fs/ntfs/inode.c 2004-11-09 11:42:05 +01:00
+++ edited/fs/ntfs/inode.c 2005-02-13 17:40:55 +01:00
@@ -388,6 +388,7 @@ void __ntfs_init_inode(struct super_bloc
ni->attr_list = NULL;
ntfs_init_runlist(&ni->attr_list_rl);
ni->itype.index.bmp_ino = NULL;
+ spin_lock_init(&ni->itype.index.bmp_ino_lock);
ni->itype.index.block_size = 0;
ni->itype.index.vcn_size = 0;
ni->itype.index.collation_rule = 0;
@@ -414,6 +415,29 @@ inline ntfs_inode *ntfs_new_extent_inode
return ni;
}
+/*
+ * Grab primary inode when we're using the attr inode because they
+ * share state.
+ */
+struct inode *ntfs_grab_bmp_inode(struct inode *inode)
+{
+ struct inode *bmp_inode;
+ ntfs_inode *ni = NTFS_I(inode);
+
+ spin_lock(&ni->itype.index.bmp_ino_lock);
+ bmp_inode = ni->itype.index.bmp_ino;
+ if (bmp_inode && !igrab(inode))
+ bmp_inode = NULL;
+ spin_unlock(&ni->itype.index.bmp_ino_lock);
+
+ return inode;
+}
+
+void ntfs_ungrab_bmp_inode(struct inode *inode)
+{
+ iput(inode);
+}
+
/**
* ntfs_is_extended_system_file - check if a file is in the $Extend directory
* @ctx: initialized attribute search context
@@ -1365,7 +1389,6 @@ static int ntfs_read_locked_attr_inode(s
* Make sure the base inode doesn't go away and attach it to the
* attribute inode.
*/
- igrab(base_vi);
ni->ext.base_ntfs_ino = base_ni;
ni->nr_extents = -1;
@@ -1651,7 +1674,6 @@ skip_large_index_stuff:
* Make sure the base inode doesn't go away and attach it to the
* index inode.
*/
- igrab(base_vi);
ni->ext.base_ntfs_ino = base_ni;
ni->nr_extents = -1;
@@ -2102,37 +2124,6 @@ err_out:
return -1;
}
-/**
- * ntfs_put_inode - handler for when the inode reference count is decremented
- * @vi: vfs inode
- *
- * The VFS calls ntfs_put_inode() every time the inode reference count
(i_count)
- * is about to be decremented (but before the decrement itself.
- *
- * If the inode @vi is a directory with two references, one of which is being
- * dropped, we need to put the attribute inode for the directory index bitmap,
- * if it is present, otherwise the directory inode would remain pinned for
- * ever.
- */
-void ntfs_put_inode(struct inode *vi)
-{
- if (S_ISDIR(vi->i_mode) && atomic_read(&vi->i_count) == 2) {
- ntfs_inode *ni = NTFS_I(vi);
- if (NInoIndexAllocPresent(ni)) {
- struct inode *bvi = NULL;
- down(&vi->i_sem);
- if (atomic_read(&vi->i_count) == 2) {
- bvi = ni->itype.index.bmp_ino;
- if (bvi)
- ni->itype.index.bmp_ino = NULL;
- }
- up(&vi->i_sem);
- if (bvi)
- iput(bvi);
- }
- }
-}
-
static void __ntfs_clear_inode(ntfs_inode *ni)
{
/* Free all alocated memory. */
@@ -2198,18 +2189,6 @@ void ntfs_clear_big_inode(struct inode *
{
ntfs_inode *ni = NTFS_I(vi);
- /*
- * If the inode @vi is an index inode we need to put the attribute
- * inode for the index bitmap, if it is present, otherwise the index
- * inode would disappear and the attribute inode for the index bitmap
- * would no longer be referenced from anywhere and thus it would remain
- * pinned for ever.
- */
- if (NInoAttr(ni) && (ni->type == AT_INDEX_ALLOCATION) &&
- NInoIndexAllocPresent(ni) && ni->itype.index.bmp_ino) {
- iput(ni->itype.index.bmp_ino);
- ni->itype.index.bmp_ino = NULL;
- }
#ifdef NTFS_RW
if (NInoDirty(ni)) {
BOOL was_bad = (is_bad_inode(vi));
@@ -2236,15 +2215,22 @@ void ntfs_clear_big_inode(struct inode *
__ntfs_clear_inode(ni);
+ /* Release the base inode if we are holding it. */
if (NInoAttr(ni)) {
- /* Release the base inode if we are holding it. */
- if (ni->nr_extents == -1) {
- iput(VFS_I(ni->ext.base_ntfs_ino));
+ ntfs_inode *base_inode = ni->ext.base_ntfs_ino;
+
+ if (S_ISDIR(VFS_I(base_inode)->i_mode)) {
+ spin_lock(&base_inode->itype.index.bmp_ino_lock);
+ base_inode->itype.index.bmp_ino = NULL;
+ iput(VFS_I(base_inode));
+ spin_unlock(&base_inode->itype.index.bmp_ino_lock);
+ ni->ext.base_ntfs_ino = NULL;
+ } else if (ni->nr_extents == -1) {
ni->nr_extents = 0;
ni->ext.base_ntfs_ino = NULL;
+ iput(VFS_I(base_inode));
}
}
- return;
}
/**
--- 1.45/fs/ntfs/inode.h 2004-10-25 11:46:30 +02:00
+++ edited/fs/ntfs/inode.h 2005-02-13 17:32:58 +01:00
@@ -101,6 +101,7 @@ struct _ntfs_inode {
struct { /* It is a directory, $MFT, or an index inode. */
struct inode *bmp_ino; /* Attribute inode for the
index $BITMAP. */
+ spinlock_t bmp_ino_lock;
u32 block_size; /* Size of an index block. */
u32 vcn_size; /* Size of a vcn in this
index. */
@@ -275,6 +276,9 @@ extern struct inode *ntfs_attr_iget(stru
extern struct inode *ntfs_index_iget(struct inode *base_vi, ntfschar *name,
u32 name_len);
+extern struct inode *ntfs_grab_bmp_inode(struct inode *inode);
+extern void ntfs_ungrab_bmp_inode(struct inode *inode);
+
extern struct inode *ntfs_alloc_big_inode(struct super_block *sb);
extern void ntfs_destroy_big_inode(struct inode *inode);
extern void ntfs_clear_big_inode(struct inode *vi);
@@ -295,8 +299,6 @@ extern ntfs_inode *ntfs_new_extent_inode
extern void ntfs_clear_extent_inode(ntfs_inode *ni);
extern int ntfs_read_inode_mount(struct inode *vi);
-
-extern void ntfs_put_inode(struct inode *vi);
extern int ntfs_show_options(struct seq_file *sf, struct vfsmount *mnt);
--- 1.184/fs/ntfs/super.c 2005-01-05 03:48:14 +01:00
+++ edited/fs/ntfs/super.c 2005-02-13 17:15:47 +01:00
@@ -2186,9 +2186,6 @@ static int ntfs_statfs(struct super_bloc
static struct super_operations ntfs_sops = {
.alloc_inode = ntfs_alloc_big_inode, /* VFS: Allocate new inode. */
.destroy_inode = ntfs_destroy_big_inode, /* VFS: Deallocate inode. */
- .put_inode = ntfs_put_inode, /* VFS: Called just before
- the inode reference count
- is decreased. */
#ifdef NTFS_RW
//.dirty_inode = NULL, /* VFS: Called from
// __mark_inode_dirty(). */
-
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
